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

ENH: Rename the csv module to prevent errors #788

Merged
merged 11 commits into from
Jan 19, 2024

Conversation

mgrover1
Copy link
Collaborator

@mgrover1 mgrover1 commented Jan 16, 2024

Rename the csv module to text

  • Closes Rename CSV submodule #787
  • Tests added
  • Documentation reflects changes
  • PEP8 Standards or use of linter
  • Xarray Dataset or DataArray variable naming follows 'ds' or 'da' naming

@zssherman
Copy link
Collaborator

@mgrover1 Looks good to me, failing test seems to be an import error still, the doc build fail is strange as we don't pin sphinx, maybe because we pip install it in the enviromnet_docs.yml and its pip installing an earlier version?

@zssherman
Copy link
Collaborator

@mgrover1 Looks like pysp2 doc is failing due to sphinx as well, wonder if sphinx did an update...

@mgrover1
Copy link
Collaborator Author

Yes, it is happening across the Pythia sphere as well

@zssherman
Copy link
Collaborator

zssherman commented Jan 16, 2024

@mgrover1 Ah makes sense.... yeah was digging through sphinx and couldn't find anything, can you reference one of the pythia issues here?

@mgrover1
Copy link
Collaborator Author

see the discussion here ProjectPythia/cookbook-template#154

@zssherman
Copy link
Collaborator

zssherman commented Jan 16, 2024

@mgrover1 Pushed an attempted fix commit for the path in noaapsl. Not sure why the path is failing there but not in neon.py etc. We might have to dig further on the cause.

@zssherman
Copy link
Collaborator

zssherman commented Jan 16, 2024

@mgrover1 found the problem think its fixed.

@zssherman
Copy link
Collaborator

Surprised that didn't fix the issue...

@mgrover1
Copy link
Collaborator Author

@zssherman - the act.io.noaapsl submodule's import was inconsistent - I pushed a fix

@zssherman
Copy link
Collaborator

zssherman commented Jan 16, 2024

@mgrover1 I added in the noaa psl change to test it out, but the init change was missing text instead of csv for the top module. Thought that would have fixed the issue when I changed that

@mgrover1
Copy link
Collaborator Author

It would help if the submodule were actually added... that last push should resolve the issues 👍

@mgrover1
Copy link
Collaborator Author

@rcjackson - this is a solid state now - do you have the PR ready to merge or do a release of PySP2

Copy link
Collaborator

@zssherman zssherman left a comment

Choose a reason for hiding this comment

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

Changes look good, approving until the release of pysp2.

@rcjackson
Copy link
Collaborator

New release of PySP2 is out on PyPI, so once that gets through these unit tests should succeed. PySP2's are succeeding on my machine with @mgrover1 's fork installed though currently failing online because this PR and release is not done.

@AdamTheisen
Copy link
Collaborator

@zssherman would you want to get a new release done today?

@zssherman
Copy link
Collaborator

@AdamTheisen Yeah, I can do that!

@zssherman
Copy link
Collaborator

@AdamTheisen An issue with the PySP2 release, going to get that fix, then can do the ACT release

@mgrover1
Copy link
Collaborator Author

Can I hit merge?

@zssherman zssherman merged commit 20f206c into ARM-DOE:main Jan 19, 2024
16 checks passed
@zssherman
Copy link
Collaborator

@mgrover1 Yep was waiting on pysp2 to catch up!

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.

Rename CSV submodule
4 participants