Why doesn't FieldGenerator have a null constructor????!#144
Why doesn't FieldGenerator have a null constructor????!#144michael-petersen merged 3 commits intomainfrom
FieldGenerator have a null constructor????!#144Conversation
michael-petersen
left a comment
There was a problem hiding this comment.
Looks good to me -- a straightforward fix with the null constructor, and always helpful to have the docs changes. Again bumping version since this is good thing to add to main (although I guess we could have made the case for adding to level, as it is sort of feature-like).
There was a problem hiding this comment.
Pull Request Overview
Adds a no-argument constructor for FieldGenerator so it can be used without supplying grid parameters, updates Python bindings, adjusts a density function’s normalization, and bumps the project version.
- Introduce
FieldGenerator()default constructor in C++ and expose via pybind11 - Update DiskDensityFunc normalization factor and exponent scaling
- Bump project version to 7.8.6
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pyEXP/FieldWrappers.cc | Added pybind11 null constructor and docstring |
| include/DiskDensityFunc.H | Adjusted exponent factor and normalization |
| expui/FieldGenerator.H | Introduced FieldGenerator() default constructor |
| CMakeLists.txt | Updated project version to 7.8.6 |
Comments suppressed due to low confidence (2)
pyEXP/FieldWrappers.cc:377
- Unresolved merge conflict markers are present here; please remove
<<<<<<<,=======, and>>>>>>>lines and reconcile the conflicting blocks.
<<<<<<< HEAD
expui/FieldGenerator.H:40
- [nitpick] Consider adding unit tests for the new default constructor to verify that methods like
points()andhisto1dlog()behave correctly without grid inputs.
FieldGenerator() = default;
|
I agree that this could be considered a new feature. But I'd argue that it's a bug of omission in the sense that member-function semantics demand a null constructor. |
|
Yes, this does seem more like bug than feature. And certainly doesn't break anything. So I'm bringing it in now with the updated version. |
Main change
Added a null constructor to
FieldGeneratorso that one can instantiate for use with non-grid features, e.g. using thepoints()member, without having to supply dummy grid values.[Some comment and docstring fixes came along for the ride.]
Details
Add the null constructor to
FieldGenerator.Hand exposed bypybind11inFieldWrappers.cc.Tested in an existing work flow that I can do:
Should work for other cases, such as
fields.points().