DM-43905: Add (modelfit_)parameters to lsst_distrib #12
Conversation
include/parameters.h
Outdated
| @@ -40,7 +40,7 @@ struct SizeYParameter : public SizeParameter { | |||
| }; | |||
|
|
|||
| template <typename T> | |||
| using Param = parameters::Parameter<double, T>; | |||
| using Param = modelfit_parameters::Parameter<double, T>; | |||
There was a problem hiding this comment.
While it's not actually forbidden, are you intending to expose gauss2d::fit::Param<T> as part of the library API? If not, I recommend dropping it and using explicit names for the base classes.
There was a problem hiding this comment.
Is the concern that users might reference gauss2d::fit::Param instead of modelfit_parameters::Parameter<double, T>? But even for a CRTP base, this is a type alias and they'd be equivalent, no?
There was a problem hiding this comment.
No, the concern is that some other code that uses Param and happens to include this file might suddenly (and surprisingly) get a collision. As you say, the namespace mitigates the risk, but it still seems iffy to expose names that aren't intended for use.
There was a problem hiding this comment.
Ok, my conclusion is that I'll renamed to Param to ParameterD in line with stack naming conventions (and current use in multiprofit). I suppose I could do this in modelfit_parameters but slightly prefer to do it here and not impose that nomenclature on other hypothetical modelfit_parameters users.
meson.build
Outdated
| @@ -26,13 +26,13 @@ if use_eups | |||
| include_directories: gauss2d_eupsdir + '/build-release/include', | |||
| ) | |||
|
|
|||
| parameters_eupsdir = run_command('eups', 'list', '-d', 'parameters', check: true).stdout().strip() | |||
| parameters_eupsdir = run_command('eups', 'list', '-d', 'modelfit_parameters', check: true).stdout().strip() | |||
There was a problem hiding this comment.
I don't think this will actually work in general -- eups list -d lists all installed versions of a given package, regardless of tag. I'd recommend relying on MODELFIT_PARAMETERS_DIR instead; I don't think there's any expectation that a package is buildable if it is not set up.
There was a problem hiding this comment.
Part of the reason I did it this way is that Meson is very opinionated about not providing access to environment variables. However, it turns you can use run_command('sh', '-c', '...') and reference environment variables in the command, which I'm not happy about but seems to be the easiest solution at the moment.
The alternative is to write a (Python) eups module for Meson, which is possible but not nearly as trivial as I'd hope.
python/gauss2d/fit/parameters.h
Outdated
| #include "parameters/transform.h" | ||
|
|
||
| #include <modelfit_parameters/limits.h> | ||
| #include <modelfit_parameters/transform.h> |
There was a problem hiding this comment.
In addition to my earlier comment about <>, please include dependencies before local includes.
There was a problem hiding this comment.
You mean:
#include "lsst/modelfit/parameters.h"
#include "gauss2d/fit/parameters.h"
... separating them because the latter file is part of this package, though I still need to include it by the full namespace?
taranu
force-pushed
the
tickets/DM-43905
branch
3 times, most recently
from
1bd43ba to
ce5df2e
Compare
taranu
force-pushed
the
tickets/DM-43905
branch
2 times, most recently
from
c9e6e62 to
4d68703
Compare
include/param_defs.h
Outdated
| #include "parameters/parameter.h" | ||
| #include "lsst/modelfit/parameters.h" | ||
|
|
||
| namespace parameters = lsst::modelfit::parameters; |
There was a problem hiding this comment.
This will cause any source file that (directly or indirectly) includes this header to define parameters. Potential namespace collision if it's also using some other parameters?
include/sersicmixcomponent.h
Outdated
| * @note See https://ned.ipac.caltech.edu/level5/March05/Graham/Graham2.html | ||
| * for a useful summary of various properties of the Sersic profile. | ||
| * (Graham & Driver 2005) for a useful summary of various properties | ||
| * of the Sersic profile. |
There was a problem hiding this comment.
Prefer inline links:
| * @note See https://ned.ipac.caltech.edu/level5/March05/Graham/Graham2.html | |
| * for a useful summary of various properties of the Sersic profile. | |
| * (Graham & Driver 2005) for a useful summary of various properties | |
| * of the Sersic profile. | |
| * @note See [Graham & Driver 2005](https://ned.ipac.caltech.edu/level5/March05/Graham/Graham2.html) | |
| * for a useful summary of various properties | |
| * of the Sersic profile. |
include/model.h
Outdated
There was a problem hiding this comment.
In the future, please do clang-format runs in a separate commit.
There was a problem hiding this comment.
I think I've finally managed to get clang-format on save working everywhere I need it, so hopefully that will obviate the need for manual runs.
There was a problem hiding this comment.
It's auto-formatting that tends to be the problem, because reformats of unrelated code get mixed in with the written-as-formatted changes.
There was a problem hiding this comment.
Right, but there shouldn't be any unrelated changes once everything is formatted unless the format rules are changed or someone commits non-compliant changes, in which case sure, I'll do a separate format step first.
No description provided.