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

Update documentation for citations and input file formats #26

Merged
merged 6 commits into from
May 24, 2023

Conversation

jkikstra
Copy link
Collaborator

@jkikstra jkikstra commented Feb 8, 2023

Updates documentation:

  • First commit: be more explicit about input data (not .xls, only .xlsx or .csv)
  • Second commit: update citations to Nicholls et al. (2022) and Kikstra et al. (2022), and fix Zenodo links where necessary

Adresses #25

@jkikstra jkikstra requested a review from phackstock February 8, 2023 11:27
@jkikstra
Copy link
Collaborator Author

jkikstra commented Feb 8, 2023

Looks like a test is failing because of a numpy update;

=========================== short test summary info ============================
FAILED tests/regression/test_workflow.py::test_workflow_ciceroscm - AssertionError: AttributeError("module 'numpy' has no attribute 'float'.
  `np.float` was a deprecated alias for the builtin `float`.

@phackstock
Copy link
Contributor

phackstock commented Feb 9, 2023

Looks like a test is failing because of a numpy update;

I guess there's two ways of fixing this, pinning the numpy version or changing np.float to float. I would say the latter is preferred.

@jkikstra
Copy link
Collaborator Author

Looks like a test is failing because of a numpy update;

I guess there's two ways of fixing this, pinning the numpy version or changing np.float to float. I would say the latter is preferred.

Yes - just need to find where it is actually used, will try and go down the rabbit hole

@phackstock
Copy link
Contributor

If you, for now, pin the version at numpy <= 1.19 it should be fine (see the deprecation section of the numpy 1.20 release notes https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations).
Once cicero addresses the numpy issue you can remove the version pin.

@jkikstra
Copy link
Collaborator Author

OK.

Have made an issue on openscm-runner; openscm/openscm-runner#81

@lewisjared
Copy link
Contributor

lewisjared commented Feb 12, 2023

Thanks for the MR and diagnosis. If the current master branch of openscm-runner fixes the tests, I'll make a new release.

@jkikstra
Copy link
Collaborator Author

Okay - thanks @lewisjared!

Let me know when we can/should update the dependency, since it was so far pinned on 0.9.1.
Maybe in a separate PR?

@jkikstra jkikstra mentioned this pull request May 11, 2023
4 tasks
@lewisjared
Copy link
Contributor

Sorry I missed this. I just cut a new release so that should be available in the next hour

@lewisjared
Copy link
Contributor

lewisjared commented May 12, 2023

Hitting a few roadblocks regarding test coverage so it might not be resolved today, but I'll fix as soon as I can

openscm/openscm-runner#83

@jkikstra jkikstra mentioned this pull request May 23, 2023
setup.cfg Outdated
@@ -35,7 +34,7 @@ install_requires =
importlib-metadata
joblib
matplotlib
numpy
numpy<= 1.19
Copy link
Contributor

Choose a reason for hiding this comment

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

Once the pin is removed and the tests run, good to merge from my side.

@jkikstra
Copy link
Collaborator Author

Adjusted, and passing tests now.

@jkikstra jkikstra requested a review from phackstock May 24, 2023 06:52
Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

Perfect, thanks @jkikstra

@phackstock phackstock merged commit 315a465 into main May 24, 2023
@phackstock phackstock deleted the minor-update-docs branch May 24, 2023 07:35
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