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

Fix jax.config, remove astropy import #20

Merged
merged 3 commits into from
May 20, 2024
Merged

Fix jax.config, remove astropy import #20

merged 3 commits into from
May 20, 2024

Conversation

dgegen
Copy link
Contributor

@dgegen dgegen commented Mar 28, 2024

Two small changes to ensure that nuance runs without errors after installation via pip.

  • from jax.config import config raises an ImportError in the newest version of JAX (0.4.25). It turns out that importing the jax.config submodule via import jax.config is deprecated and referencing the config object via jax.config is encouraged. Thus, config = jax.config was added to __init__.py, so that nuance.config can still be used to access jax.config as was the case previously, although one could probably debate whether this is desirable conceptually.
  • star.py imports import astropy.units as u, but astropy was not listed as a dependency in pyproject.toml.

Keep up the inspiring work!

- `from jax.config import config` raised an ImportError in JAX 0.4.25.
  Specifically: Importing the jax.config submodule via
  `import jax.config` is deprecated. To configure JAX use `import jax`
  and then reference the config object via `jax.config`.
- `star.py` imports `import astropy.units as u`, but astropy was not
  listed as a dependency in `pyproject.toml`.
@lgrcia
Copy link
Owner

lgrcia commented Mar 29, 2024

Hi @dgegen and thanks for the changes, both relevant. astropy is imported but astropy.units is actually not used. That's intentional, to avoid having it as a dependency. Could you remove the import and keep astropy out of the dependencies? Otherwise good catch on the new jax version.

@dgegen
Copy link
Contributor Author

dgegen commented Mar 29, 2024

Thanks!

Regarding astropy: I'm afraid I've run into problems without adding astropy as an additional dependency: If I install Nuance from GitHub into a fresh environment and then only change the jax.config lines in the __init__.py file, I face the following error:

>>> import nuance
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/usr/pydev/nuance/nuance/nuance/__init__.py", line 11, in <module>
    from nuance.star import Star
  File "/Users/usr/pydev/nuance/nuance/nuance/star.py", line 3, in <module>
    import astropy.units as u
ModuleNotFoundError: No module named 'astropy'

@lgrcia
Copy link
Owner

lgrcia commented Mar 29, 2024

Sure, it makes sense since it is imported in this file. Unless I am missing something I think you can remove the import in star.py (https://github.com/lgrcia/nuance/blob/main/nuance/star.py#L3)

@dgegen
Copy link
Contributor Author

dgegen commented Mar 29, 2024

Oh my – I am being silly again! For some reason I had thought you just wanted me to take out the dependency, but re-reading it now, I see that you clearly said to take out the import as well 🙈 Sorry for the inconvenience – writing your last message must have been a bit disturbing :))

@lgrcia
Copy link
Owner

lgrcia commented Mar 29, 2024

No worries at all :) Thanks a lot for these additions!

@lgrcia
Copy link
Owner

lgrcia commented Mar 29, 2024

Oh one last thing before merging. Can you bump the minor version of the package in the pyproject file? It should be 0.6.1 now I think.

@dgegen
Copy link
Contributor Author

dgegen commented Mar 29, 2024

Done!

@dgegen dgegen changed the title Fix jax.config, add astropy as dependency Fix jax.config, remove astropy import Mar 29, 2024
@lgrcia lgrcia merged commit bb804fa into lgrcia:main May 20, 2024
1 check passed
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.

2 participants