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

Py separation #147

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

Py separation #147

wants to merge 133 commits into from

Conversation

gudlot
Copy link

@gudlot gudlot commented Nov 30, 2022

Here is my latest version for the NURF project.

There are 4 .py files

  • plot.py: Plot functions
  • fluo.py: Fluo-related functions
  • utils.py: General functions applicable to both exp. methods
  • uv.py: UV-related functions
    There are three .ipynb for demonstration, that need more cleaning, and the SANS version needs more TLC.
    -fluo_demonstration.ipynb
    -uv_demonstration.ipynb
    -SANS_demonstration.ipynb

There is as well:

  • ill_auxilliary_funcs.py: Contains nurf_file_creator function that outlines how NURF data should be organised in Nexus files.
  • ill_to_loki.ipynb: Is the jupyter notebook based on ill_auxilliary_funcs.py
    A future NURF module at ESS does not need these files because you will have your own Nexus files, but I provide them because the test files from ILL require them to convert the data to the new format.

RGBYCP and others added 30 commits June 10, 2022 23:19
@wpotrzebowski
Copy link
Contributor

@gudlot it seems that this pull request considerably overlaps with the older one (#129). Should #129 be closed or still need to be reviewed and merged?

@gudlot
Copy link
Author

gudlot commented Oct 18, 2023

Sorry, I have not seen your comment @wpotrzebowski earlier.
Here I implemented the changes that were suggested in #129 I would suggest continuing from here. I don't know what is the current status of this project.

I don't check for int because scipp does it.
"""

if not ('spectrum' and 'wavelength') in da.dims:

Choose a reason for hiding this comment

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

This condition only checks for the presence of 'wavelength'.

>>> spectrum = {'spectrum': 1}
>>> wavelength = {'wavelength': 1}
>>> both = {'spectrum': 1, 'wavelength': 1}
>>> print(('spectrum' and 'wavelength') in spectrum)
False
>>> print(('spectrum' and 'wavelength') in wavelength)
True
>>> print(('spectrum' and 'wavelength') in both)
True

I think you meant to write if 'spectrum' not in da.dims or 'wavelength' not in da.dims

Copy link
Author

Choose a reason for hiding this comment

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

Hi @dmytrokyrychuk
Thank you very much for pointing this out. I fell for a syntax that Python does not support. I replaced your suggestion with the issubset()-method:

if not {'spectrum', 'wavelength'}.issubset(da.dims):
        raise ValueError('Dimensions spectrum and wavelength expected.')

What do you think about this?
You are correct. I want both spectrum and wavelength as dimensions in the scipp.DataArray. Your suggestion would work, of course, too.
I should have implemented tests, but my contract finished.

In addition, I fixed changes in the API of scipp, in utils.py, fluo.py, 'uv.py'. But on the TODO list remain now some issues in the plot.py.

Choose a reason for hiding this comment

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

Your suggestion is valid. I like my suggestion because it reads like a sentence.

Please note that I am not affiliated with this project. I was simply stumbled upon this PR and reported an issue I noticed.

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

Successfully merging this pull request may close these issues.

None yet

4 participants