Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add field static_phi_list to NXxpcs #1314

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

prjemian
Copy link
Contributor

@prjemian prjemian commented Oct 11, 2023

@AZjk
Copy link

AZjk commented Oct 13, 2023

static_phi_list is necessary for the result file. thank you for adding that.

Copy link
Contributor

@padraic-shafer padraic-shafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor comments here:

  • There seems to be an inconsistent use of q and Q. Is this meaningful or arbitrary?
  • Find-replace for 'xcps' -> 'xpcs'

contributed_definitions/NXxpcs.nxdl.xml Outdated Show resolved Hide resolved

The ``units`` attribute should be set to ``"au"``
indicating arbitrary units.
</doc>
</field>
<field name="static_q_list" type="NX_NUMBER" units="NX_PER_LENGTH" minOccurs="0">
<doc>
1-D list of :math:`|Q|` values, 1 for each roi.
1-D list of :math:`|Q|` values, one for each roi.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this doc field describe the same options as in its dynamic counterpart?

  • Q(r)
  • Q(r), phi
  • H, K, L
  • qx, qy, qz
  • x, y pixels

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! You are correct about the inconsistent use of Q and q. In small-angle (which is used together sometimes with XPCS), $Q$ has a different meaning than reciprocal space vector; it represents the total scattered intensity, integrated over all $\vec{q}$. I will suggest a revision as part of this PR. Should that discussion become distracting to the intent of this PR (as stated in the title), I suggest moving such comment and revision to a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sureshnaps, @AZjk, @qzhang234, @ambarb, @AbbyGi: Can you comment if the options and other documentation for dynamic_q_list should be copied to static_q_list?

Copy link

@ambarb ambarb Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this very descriptive option of all possible q-notations was left out of static_q_list on purpose because of complexity. Historically, the static_q_list (and now added static_phi_list) are used by the the team at the APS to describe the non-dynamical SAXS.

In the early development phase of NXxpcs, there was a proposal to not include these (the static lists) as part of the NXxpcs definition but to "add on" NXcanSAS. Eventually, we (the scattering community at large) could then contribute new NX definitions for describing the scattering response for GI-SAXS and single crystal diffraction geometries. Ultimately, this is not where compromise was reached in the effort to suggest a NXxpcs definition since none of the participants heavily using XPCS SAXS were using NXcanSAS. The static_q_list was consequently added to the NXxpcs definition.

Why only SAXS? The data types for the non-SAXS could be very much more complicated and perhaps it is easier to just limit this to SAXS. V2 could be to expand this as more non-SAXS progresses and there should probably be a workshop for this.

For instance, HKL is may be addressed best as a 4D array or some X-array that contains each pixel H, K, L, and Intensity. But this is the least useful form. What people would want is the width in user specified directions and integrated intensity overall. Or perhaps others want to include the the data from the rocking curve so one would not just have the average image of the HKL for XPCS but a volume through the q-space region.

As I understood it, this static data have a much finer resolution (but not as high as a beamline built to focus on uSAXS). Therefore, the static_q_list and dyanamic_q_list do not match with the roi_map utilized for the g2 and C2 computations of XPCS. @AZjk please confirm.

I would suggest documenting further that this is limited to SAXS and explaining what the data should be. The open questions would be:

- is it strictly the highest q-resolution possible? if not, do you need an roi_map?
- is it sourced from the average of all XPCS frames or is it a time resolved result?
- what is the difference between the static_q_list and the static_phi_list (same length or is q_list only Q(r)? if the later, how does one know which Q(r) corresponds to the phi_list )?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @prjemian, per your request, the APS folks have discussed and we think it would be nice to fold phi_list into the q_list. We imagine that there will be a static_q_list and dynamic_q_list, and each list will contain several columns corresponding to, for example, q, phi, qx, qy, qz, h, k, l, so on and so forth.

Each scattering geometry will probably use only a few of these columns, so it would be useful to also come up with some dummy/default values to avoid confusion.

In the future we could further expand this to q_parallel, q_vertical, etc., to include the GIXS geometry as well.

Would this sound like an acceptable plan @padraic-shafer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@qzhang234 Thanks for the gathering input from APS folks and sharing the consensus here!

I think that makes sense to fold these into single lists--one for static and one for dynamic. @ambarb: Is this what you were thinking also?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since many of those columns (q, phi, qx, qy, qz, h, k, l, etc.) would be empty if we insist that all of them be present...@ambarb had suggested that we could instead add a @storage_mode attribute.

This would be similar to how it is used in the NXxpcs::entry::twotime group, and it could have defined values, such as q_only, q_phi, qxyz, hkl, xy, or whatever makes sense here. Effectively this would act as a tagged union discriminator for the q_lists. I think this sounds useful.

This could be split out into a separate PR, but it seemed relevant to bring it up here. What do others think?

Copy link

@ambarb ambarb Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since many of those columns (q, phi, qx, qy, qz, h, k, l, etc.) would be empty if we insist that all of them be present...@ambarb had suggested that we could instead add a @storage_mode attribute.

This would be similar to how it is used in the NXxpcs::entry::twotime group, and it could have defined values, such as q_only, q_phi, qxyz, hkl, xy, or whatever makes sense here. Effectively this would act as a tagged union discriminator for the q_lists. I think this sounds useful.

This could be split out into a separate PR, but it seemed relevant to bring it up here. What do others think?

@padraic-shafer this is what I am thinking. This way we can reduce writing empty fields that do not have to be individually opened to confirm there is NO data in that list. GUI's and other code can just look at the attribute and know what kind of data to expect.

as for as the names go, I like the ones you propose. I would suggest a little change for consistency and not for someone to confuse what xy might be.

q_r, q_phi, q_rphi, q_xyz, hkl, pixel_xy

I suggest q_r, q_phi and redundantlyq_rphi because I do not know if what the APS wants for q_r, q_phi. @qzhang234 are q_r, q_phi the same length? If yes, maybe we can keep them separate and remove q_rphi, but we should probably understand the advantage of this choice from your perspective.

I agree with @qzhang234 about holding off on "q_parallel, q_vertical, etc" until we finalize this and see how this works "in the wild".

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one thing not yet resolved is how the static_q_list is envisioned to be used. I imagine that the list correlates an intensity list so that one may plot I(Q). @qzhang234 can you confirm?

The exact wording in our current NXxpcs definition for static and dynamic q-labels is:

**"Dynamic” represents quantities directly related to XPCS and NXxcps/entry/data and NXxpcs/entry/two_time.

“Static” refers to finer binning used for computation not strictly used for the final XPCS results. Implementation of static binning is left for individual libraries to document. We encourage usage of NXcanSAS to represent standard SAXS results or development of new NeXus definitions for GI-SAXS or other reciprocal space intensity mapping.**

I was wrong, there is a static_roi_map so this part is handled, but I see absolutely no data to plot against the x-variables (static_q_list or static_phi_list). I believe this is because we left it to others to document as the preference was to use NXcanSAS. I am fine with what the community wants to do in this regard (I(Q) fully inside custom NXxpcs versus NXcanSAS), but if the choice is fully inside NXxcps, it seems better to fully document beyond just adding "static_phi_list" and the other discussed changes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since many of those columns (q, phi, qx, qy, qz, h, k, l, etc.) would be empty if we insist that all of them be present...@ambarb had suggested that we could instead add a @storage_mode attribute.

This would be similar to how it is used in the NXxpcs::entry::twotime group, and it could have defined values, such as q_only, q_phi, qxyz, hkl, xy, or whatever makes sense here. Effectively this would act as a tagged union discriminator for the q_lists. I think this sounds useful.

This could be split out into a separate PR, but it seemed relevant to bring it up here. What do others think?

#1314 (comment). should have been in this thread, not where it is at the bottom of this page

prjemian and others added 2 commits October 23, 2023 18:26
Co-authored-by: Padraic Shafer <76011594+padraic-shafer@users.noreply.github.com>
* iterable list of tuples (i.e., :math:`Q(r)`, :math:`\varphi`), but preferable use the seperate :math:`\varphi` field below
* iterable list of tuples (e.g., (H, K, L); (qx, qy, qz); (horizontal_pixel, vertical_pixel))
* iterable list of tuples (i.e., :math:`Q(r)`, :math:`\varphi`), but preferable use the separate :math:`\varphi` field below
* iterable list of tuples (e.g., :math:`(H, K, L)`; :math:`(q_x, q_y, q_z)`; (horizontal_pixel, vertical_pixel))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prjemian I understood recently from CHX, they use a list of arrays for HKL

Is it worth a discussion or should we stick to this (tuples) ?

Copy link

@ambarb ambarb Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the dynamic_q_list and static_q_list allow for generic implementation of any reciprocal space notation. It seems confusing that there is a phi_list of any kind give the description for q_list.

If phi_lists are a requirement, then we need to develop more guidance on how to use both options to notate any phi dependance to XPCS results (q_list - a list of tuples vs q_list AND phi_list (of which I am unable to describe).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some history regarding the origin of q_list: another data type to track Q as a function of roi_map pixel values is to use a python dictionary in which the format is a nested dictionary with key, value pairs of {row_map_pixel_value:reciprocal_space_quantity}. Converting this to an iterable q_list seemed reasonable given that a list was the requested format by others in the NXxpcs origination team.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the best place for this suggestion as there are concerns on how to make things more clear in terms of the notations used for q (within the q_lists and against the phi_list

To make the usage of q_list absolutely clear and deal with some of the ambiguity in the q-space notation, let's add an attribute with the allowed values of descriptive text.

  • Q(r) --------> attribute value = "radial"
  • Q(r , phi) ---> attribute value = "radial with azimuth" or "radial with phi" or something else @AZjk or @sureshnaps or @qzhang234 can suggest
  • H, K, L -----> attribute value = "hkl"
  • qx, qy, qz --> attribute value = "qxyz"
  • x, y pixels --> attribute value = "pixels"

@AZjk or @sureshnaps or @qzhang234 , for the phi_list can you please describe if this is the same length as your q_list or do you always have one bin for each q-radius? Am I correct in that the Q(r) resolution is limited to your pixel size and that you use the highest resolution possible?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@padraic-shafer suggestions for attribute names I think are better than what I posted here.


The ``units`` attribute should be set to ``"au"``
indicating arbitrary units.
</doc>
</field>
<field name="static_q_list" type="NX_NUMBER" units="NX_PER_LENGTH" minOccurs="0">
<doc>
1-D list of :math:`|Q|` values, 1 for each roi.
1-D list of :math:`|Q|` values, one for each roi.
Copy link

@ambarb ambarb Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to add a static phi list, then do we need to describe how/where these angles are defined? I imagine there is some variabilitiy. Or do we strictly rely on the dynamic_roi_map and the static_roi_map?

@AZjk maybe you can make a separate issue in this repo that describes your system of reference and then reply to the review with a link to said issue if we decide there is a need to include how the phi slices are labled?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that any phi_list is described as
starting with 1 according to the associated roi_map.

I am also a bit confused on how to map phi and q to the same dynamic_roi_map or static_roi_map, unless the q_lists and the phi_lists are of the same length...and in that case it seems redundant

@@ -507,7 +507,7 @@
roi index array or labeled array

The values of this mask index (or map to) the :math:`Q` value from the
the ``dynamic_q_list`` field. Not that the value of ``0`` represents in-action. XPCS computations
``dynamic_q_list`` field. Not that the value of ``0`` represents in-action. XPCS computations
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Note that the value of ... not Not.....


The ``units`` attribute should be set to ``"au"``
indicating arbitrary units.
</doc>
</field>
<field name="static_q_list" type="NX_NUMBER" units="NX_PER_LENGTH" minOccurs="0">
<doc>
1-D list of :math:`|Q|` values, 1 for each roi.
1-D list of :math:`|Q|` values, one for each roi.
Copy link

@ambarb ambarb Oct 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this very descriptive option of all possible q-notations was left out of static_q_list on purpose because of complexity. Historically, the static_q_list (and now added static_phi_list) are used by the the team at the APS to describe the non-dynamical SAXS.

In the early development phase of NXxpcs, there was a proposal to not include these (the static lists) as part of the NXxpcs definition but to "add on" NXcanSAS. Eventually, we (the scattering community at large) could then contribute new NX definitions for describing the scattering response for GI-SAXS and single crystal diffraction geometries. Ultimately, this is not where compromise was reached in the effort to suggest a NXxpcs definition since none of the participants heavily using XPCS SAXS were using NXcanSAS. The static_q_list was consequently added to the NXxpcs definition.

Why only SAXS? The data types for the non-SAXS could be very much more complicated and perhaps it is easier to just limit this to SAXS. V2 could be to expand this as more non-SAXS progresses and there should probably be a workshop for this.

For instance, HKL is may be addressed best as a 4D array or some X-array that contains each pixel H, K, L, and Intensity. But this is the least useful form. What people would want is the width in user specified directions and integrated intensity overall. Or perhaps others want to include the the data from the rocking curve so one would not just have the average image of the HKL for XPCS but a volume through the q-space region.

As I understood it, this static data have a much finer resolution (but not as high as a beamline built to focus on uSAXS). Therefore, the static_q_list and dyanamic_q_list do not match with the roi_map utilized for the g2 and C2 computations of XPCS. @AZjk please confirm.

I would suggest documenting further that this is limited to SAXS and explaining what the data should be. The open questions would be:

- is it strictly the highest q-resolution possible? if not, do you need an roi_map?
- is it sourced from the average of all XPCS frames or is it a time resolved result?
- what is the difference between the static_q_list and the static_phi_list (same length or is q_list only Q(r)? if the later, how does one know which Q(r) corresponds to the phi_list )?


The ``units`` attribute should be set to ``"au"``
indicating arbitrary units.
</doc>
</field>
<field name="static_q_list" type="NX_NUMBER" units="NX_PER_LENGTH" minOccurs="0">
<doc>
1-D list of :math:`|Q|` values, 1 for each roi.
1-D list of :math:`|Q|` values, one for each roi.
Copy link

@ambarb ambarb Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since many of those columns (q, phi, qx, qy, qz, h, k, l, etc.) would be empty if we insist that all of them be present...@ambarb had suggested that we could instead add a @storage_mode attribute.

This would be similar to how it is used in the NXxpcs::entry::twotime group, and it could have defined values, such as q_only, q_phi, qxyz, hkl, xy, or whatever makes sense here. Effectively this would act as a tagged union discriminator for the q_lists. I think this sounds useful.

This could be split out into a separate PR, but it seemed relevant to bring it up here. What do others think?

@padraic-shafer this is what I am thinking. This way we can reduce writing empty fields that do not have to be individually opened to confirm there is NO data in that list. GUI's and other code can just look at the attribute and know what kind of data to expect.

as for as the names go, I like the ones you propose. I would suggest a little change for consistency and not for someone to confuse what xy might be.

q_r, q_phi, q_rphi, q_xyz, hkl, pixel_xy

I suggest q_r, q_phi and redundantlyq_rphi because I do not know if what the APS wants for q_r, q_phi. @qzhang234 are q_r, q_phi the same length? If yes, maybe we can keep them separate and remove q_rphi, but we should probably understand the advantage of this choice from your perspective.

I agree with @qzhang234 about holding off on "q_parallel, q_vertical, etc" until we finalize this and see how this works "in the wild".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NXxpcs: static_phi_list missing
5 participants