Skip to content

Conversation

@Nush395
Copy link
Contributor

@Nush395 Nush395 commented Jan 20, 2025

This change involves a few pieces.

  • Support running all tests without access to IMAS core.
  • Minimal support for Numpy v2 when running with netcdf pathway.
  • Support netcdf version >= 1.4.1 for the netcdf pathway.

Copy link
Collaborator

@maarten-ic maarten-ic left a comment

Choose a reason for hiding this comment

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

Hi Nush,

Thanks for this PR!

Responding to your questions / TODO:

  • For me it's fine to put these in a single PR.

  • I've tested against IMAS core: all is working fine.

  • netCDF4 1.4.1 misses the auto_complex feature, which we require for storing complex values. This was added in 1.7.0.

    Storing complex values is not very common, so I'd be okay if we relax the netCDF minimum version in the pyproject.toml if there's a proper error message when the user attempts to store complex values in the netCDF file. Most logical place for this error would be in ids2nc.py inside create_variables().

    The current error (for netCDF4 1.6.5) is TypeError: Illegal primitive data type, must be one of dict_keys(['S1', 'i1', 'u1', 'i2', 'u2', 'i4', 'u4', 'i8', 'u8', 'f4', 'f8']), got complex128 (variable 'distribution.markers.orbit_integrals.values_error_upper', group '0') but that doesn't directly tell the user what's the problem. Doesn't this error pop up in your pytest runs for the imaspy/test/test_nc_autofill.py tests?

And then some nitpicking :)

  1. Could you please run black (>=24,<25) to autoformat your changes?
  2. Could you please run flake8 and check that it doesn't complain?

These checks will eventually be run by CI (see ci/linting.sh), but that's not configured yet on GitHub.

Cheers,
Maarten

Nush395 and others added 2 commits January 21, 2025 10:27
Co-authored-by: Maarten Sebregts <110895564+maarten-ic@users.noreply.github.com>
Co-authored-by: Maarten Sebregts <110895564+maarten-ic@users.noreply.github.com>
@Nush395
Copy link
Contributor Author

Nush395 commented Jan 21, 2025

And then some nitpicking :)

Could you please run black (>=24,<25) to autoformat your changes?
Could you please run flake8 and check that it doesn't complain?

Of course! Would it be sensible to add these to the contributing docs? Happy to add that if so!

@Nush395
Copy link
Contributor Author

Nush395 commented Jan 21, 2025

The current error (for netCDF4 1.6.5) is TypeError: Illegal primitive data type, must be one of dict_keys(['S1', 'i1', 'u1', 'i2', 'u2', 'i4', 'u4', 'i8', 'u8', 'f4', 'f8']), got complex128 (variable 'distribution.markers.orbit_integrals.values_error_upper', group '0') but that doesn't directly tell the user what's the problem. Doesn't this error pop up in your pytest runs for the imaspy/test/test_nc_autofill.py tests?

Indeed it does, I was going to ask Daan about this today so thanks for the explanation. I will add a sensible error message.

@maarten-ic
Copy link
Collaborator

And then some nitpicking :)
Could you please run black (>=24,<25) to autoformat your changes?
Could you please run flake8 and check that it doesn't complain?

Of course! Would it be sensible to add these to the contributing docs? Happy to add that if so!

It is explained in the sphinx documentation: https://github.com/iterorganization/imas-python/blob/develop/docs/source/code_style.rst but since that's not yet published I won't blame you for not knowing this 😉

Could be useful to refer to this in the contributing guidelines indeed! @olivhoenen what's your take on this?

@Nush395
Copy link
Contributor Author

Nush395 commented Jan 21, 2025

Addressed comments and updated PR, PTAL :)

@Nush395 Nush395 mentioned this pull request Jan 21, 2025
@olivhoenen
Copy link
Collaborator

Of course! Would it be sensible to add these to the contributing docs? Happy to add that if so!

It is explained in the sphinx documentation: https://github.com/iterorganization/imas-python/blob/develop/docs/source/code_style.rst but since that's not yet published I won't blame you for not knowing this 😉

Could be useful to refer to this in the contributing guidelines indeed! @olivhoenen what's your take on this?

The doc is almost there https://imas-python.readthedocs.io/en/latest/ ;-)
But I guess a very concise amount of redundancy in the contributing guidelines won't harm.

@maarten-ic maarten-ic mentioned this pull request Jan 21, 2025
Copy link
Collaborator

@maarten-ic maarten-ic left a comment

Choose a reason for hiding this comment

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

LGTM!

@maarten-ic maarten-ic requested a review from olivhoenen January 22, 2025 07:57
Copy link
Collaborator

@olivhoenen olivhoenen left a comment

Choose a reason for hiding this comment

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

LGTM

@olivhoenen olivhoenen merged commit 68c225d into iterorganization:develop Jan 22, 2025
@Nush395
Copy link
Contributor Author

Nush395 commented Jan 22, 2025

Thanks both for the rapid review on this.

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.

3 participants