Update NXparameter documentation

[rayosborn]

The name of this base class suggests that it would be the ideal repository to store the results of any parameter optimization, such as least-squares fitting. To facilitate its use in this context, it is helpful to standardize a number of attributes for each parameter, which are common to most, if not all, optimization procedures. For a number of years, NeXpy has been using NXparameter groups to define the parameters for a specific function, such as a Gaussian, in conjunction with the LMFIT package (https://lmfit.github.io/lmfit-py/), which calls them "models." If multiple functions are fitted simulaneously to a data set, multiple NXparameter groups are stored in a NXprocess group, which also defines the program used to produce the optimization.

An example of a NXparameter group is given here:

fit:NXprocess
  parameters:NXparameters
    @model = 'Lorentzian'
    amplitude = 1301.0084322223488
      @error = 83.04682658748045
      @initial_value = 1918.64
      @max = 'inf'
      @min = '-inf'
      @vary = True
   center = 109.90841596329723
      @error = 0.151393986221809
      @initial_value = 109.5
      @max = 'inf'
      @min = '-inf'
      @vary = True
.
.
.

The purpose of this PR is not to confine NXparameter groups to this particular use case, but to standardize the attributes when it applies. Others are welcome to suggest other uses of this group.

Until now, the NXparameter group has been an empty container, so I don't think this PR changes the standard in any substantive way - it just expands the documentation - so I don't believe a NIAC vote is required. However, it is probably still worth discussion at a Telco to get feedback.

[PeterC-DLS]

It's worth taking into consideration the MPES proposal where there's NXfit, NXfit_function and other base classes

[lukaspie]

It's worth taking into consideration the MPES proposal where there's NXfit, NXfit_function and other base classes

I think the various attributes that are added here like @error, @initial_value, @max, @min, @vary would greatly improve NXfit and the associated base classes. What's a bit different to how we have implemented is the additional @model attribute for which we are using NXfit_function within NXfit, so that may potentially be redundant.

[mkuehbach]

Indeed there are similarities between NXfit and suggestions made here. Personally, I think though that NXparameters should intentionally not restrict itself to fitting only but rather be a base class for where people can store parameters of algorithms irrespective of which type of algorithms these are. NXfit is maybe a specialization of NXparameters as an idea?

I see your point Ray in that you I think want to name lmfit just as an example to make the docstring less abstract for users. Maybe adding one more example for parameters other than for fitting purposes would be good to make the higher generality of NXparameters clearer.

[phyy-nx]

@rayosborn to consider if the new NXfit class serves the same function so it's not worth updating NXparameters.

Also update doc here so it's clear that fitting is just an example use case.

[mkuehbach]

A reasonable set of changes

[lukaspie]

These attributes are very helpful and I support adding them to the PARAMETER field in the base class. As discussed, I would just suggest that we align the attributes used in NXfit_function that basically do the same: https://github.com/nexusformat/definitions/blob/main/base_classes/NXfit_function.nxdl.xml#L58

Would be happy to adjust NXfit_function accordingly if we agree on the names of these attributes.
We should probably end up with attributes like these:

• description (as in NXfit_function)
• initial value
• min_value / min (we just choose one name)
• max_value / max (we just choose one name)
• vary / fixed (one of these)

[phyy-nx]

@lukaspie will make a PR that points to this PR to fix NXfit_function to use vary instead of fixed
@rayosborn will change min/max to min_value/max_value and fix the conflicts

[lukaspie]

see #1594

[mkuehbach]

A reasonable set of changes

[phyy-nx]

Awaiting the conflicts being fixed

[woutdenolf]

Personal opinion: we use NXparameters for a wide range of storing a bag of parameters, often not related to fitting. This seems to be more for NXfit.

[zdemat]

just in written what was mentioned in the meeting: I like the examples and the use for the refinable parameters in the fitting applications but my concerns are that the documentation looks too much oriented to the fitting now. However just a simple comment that it can be used in the original sense as a container for any parameters (including static program parameters) would resolve my concerns.

[phyy-nx]

From Telco: add a comment that NXparameters is very general, and that the fitting parameters are here for convenience. NXparameters is often used to hold parameters used in program execution that are not refined, for example.

[rayosborn]

I amended the documentation to add:

"Although this base class can be used to store any kind of parameter, one possible use case is to store parameters that are refined by a fitting function or model. A number of attributes have been defined to store metadata associated with such a refinement."

I hope this satisfies those who are concerned that viewers might think the group is exclusively for fitting parameters.

[phyy-nx]

As discussed on the Telco, this PR is ready for a vote, with the note that we are waiting on a change from @lukaspie to change NXfit_function to use vary instead of fixed. Update: this vote includes #1594 from @lukaspie

Please vote using emojis, 👍 for yes, 👎 for no, and anything else e.g. 👀 for abstain. Voting will close in 2 weeks.

Thanks!

[phyy-nx]

This vote includes #1594, as noted in the original vote post

[PeterC-DLS]

I thought during the telco Markus objected to the *_value names and that it was better to use min, max, and initial.

[mkuehbach]

I thought during the telco Markus objected to the *_value names and that it was better to use min, max, and initial.

Indeed, I asked for "_value" be dropped.

"min", "max" and other suggestive names for number quantities which are essentially summary statistics could be made a common for quantities. My suggestion is --- solely to support not having the release date for NXDL2025 blocked --- accept the PR as is and make edits a future issue for e.g. NXDL2026.

[PeterC-DLS]

@mkuehbach the problem is that there will be files written using these names - do you deprecated the names straight away? It's worth getting this in a reasonable shape for the release.

[rayosborn]

I appreciate your concern, @PeterC-DLS, but the two people, who might be affected with any backward-compatibility issues, i.e., @mkuehbach and me, both support this PR. I'm not sure either of us would be helped by delaying this any further. Some NeXus issues hang around for years and never get resolved by the NIAC; I would prefer this is not one of them.

[mkuehbach]

As discussed on the Telco, this PR is ready for a vote, with the note that we are waiting on a change from @lukaspie to change NXfit_function to use vary instead of fixed. Update: this vote includes #1594 from @lukaspie

Please vote using emojis, 👍 for yes, 👎 for no, and anything else e.g. 👀 for abstain. Voting will close in 2 weeks.

Thanks!

2w passed but less than 14 votes...

(...) I'm not sure either of us would be helped by delaying this any further. Some NeXus issues hang around for years and never get resolved by the NIAC; I would prefer this is not one of them.

I agree with @rayosborn

Question for me is though --- given insufficient votes --- what will happen now???
1.) Will we just accept this PR and #1594 as is?
2.) Is there another round of votes for this PR and #1594 as they are right now?
3.) Should we just remove the _value suffix from here and in #1594 for the concepts?
min_value, max_value, and initial_value, respectively and will this then need a second voting round?
4.) like 3.) but does not need a second voting round?

NIAC please decide

[phyy-nx]

The PR did not get enough votes. Interested parties should confer and get the pull requests aligned with all edits merged in before we try again.

[mkuehbach]

@phyy-nx @rayosborn @PeterC-DLS me and Lukas had a chat about this PR and #1594.

We concluded the following changes:
1.) We dropped the suffix _value in the here exemplified attributes for PARAMETER.
2.) Until Oct, 20th the CI for #1560 was still running fine, no longer thereafter for one reason, on that day a contributed definition started using the here proposed liberation of NXparameters and added a concept model as a child of an instance of NXparameters group, specifically NXmicrostructure_score_config concept /ENTRY/deformation/model
Upon running the CI from then onwards, sphinx complains about references not found for what it shouldnt.

I suspect that the documentation generation code does not properly distinguish between an attribute and field in a group when these have the same name --- something which is possible and distinguishable though.

That example here shows two concepts in such a group setting that are different and also with HDF5 no problem there is to distinguish them. The issue is general, concept names here only an example showing the problem.

deformation/@NX_class = "NXparameters"
deformation/@model = "lmfit"
deformation/model = "lmfit"

For the reason to avoid this rabbithole of digging up the documentation code and also for the reason that I am convinced model can equally useful be a field in NXparameters, I moved model from attribute to field in 478eb5a.

With this the CI runs fine, #1560 and #1594 are ready for a new voting.
@phyy-nx @rayosborn @PeterC-DLS is this a compromise to support?.

The key question remains is this a voting to take place still for NXDL2025 ?

Please see attached a manifestation of the above-mentioned bug in the documentation building process:

[phyy-nx]

From Telco, drop this field. Let application definitions define model as a PARAMETER if they want to.

[phyy-nx]

Unless @rayosborn you want model for your own work? If so, let's move it back to an attribute and fix the underlying sphinx issue. @mkuehbach will write an separate issue for the sphinx bug.

[zdemat]

We are using NXparameters in recently accepted in NXazint1d with a comment: "Parameters should exactly match those required by the algorithm used in the processing. For example, azint requires error_model, mask, n_splitting, poni, etc. There is not an explicit conflict but if any application has a parameter model, with different meaning, we have a conflict.

But I have no objections, if it is an attribute.

[rayosborn]

I have reverted to defining "model" as an attribute. If I understood the above comments, that is the preferred option and it conforms to how we currently use it in NeXpy.

[phyy-nx]

@rayosborn to make the next vote easier, maybe add the commit from #1594 into this branch on your fork so that we only need one vote? Then we can close #1594.

When that is resolved, the sphinx bug is fixed, and the model term is addressed from the above comment, we will immediately run a new vote with the goal of getting this into a patch release.

[lukaspie]

@rayosborn to make the next vote easier, maybe add the commit from #1594 into this branch on your fork so that we only need one vote? Then we can close #1594.

When that is resolved, the sphinx bug is fixed, and the model term is addressed from the above comment, we will immediately run a new vote with the goal of getting this into a patch release.

@rayosborn would you be able to bring these changes from #1594 into this branch? I would do it myself, but I can't because this branch right here is located in your forked repo where I don't have access.

[rayosborn]

I have uploaded a revised version of the NXfit_function base class with attributes duplicated in the NXparameter base class removed.