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.

Sign up for GitHub

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

Jump to bottom

Adaptation to blueprint + Update to Python 3.10 #27

Merged
merged 28 commits into from
Oct 20, 2023
Merged

Adaptation to blueprint + Update to Python 3.10 #27

merged 28 commits into from
Oct 20, 2023

Conversation

Karko93
Copy link
Contributor

@Karko93 Karko93 commented Oct 5, 2023

  • Embedded Python blueprint into pyflexplot
  • Updated Readme
  • Updated to Python 3.10.12
  • Updated reference material for the slow tests

pirmink and others added 26 commits February 23, 2023 16:19
@pirmink
Switched to copier template.
04f8275 
Note: environment.yml not yet updated, linter configs not yet merged.
@pirmink
Adapted linter config.
e25cf11
@pirmink
Fix some linter issues and temporarily disable pylint.
5b5dc31
@pirmink
Removed obsolete Makefile.
d3b5bec
@pirmink
Updated environment.
302a0dd
@pirmink
Updated pyproject.toml to use Python 3.9 as required by pyflexplot. R…
98f9c10 
…eformatted README.meteoswiss from rst to md format.
@pirmink
First step towards newer Python with unpinned requirements.
ab4d724
@twicki
fix on dataclasses wrapper
f2c7d10
@pirmink
Merge pull request #24 from twicki/new-blueprint
c5477f6 
fix on dataclasses wrapper
@pirmink
Added working environment.yml: using unpinned installation does curre…
872adf0 
…ntly not yield a working environment.
@pirmink
Updated to new blueprint using copier.
2e9d6c6
@pirmink
Merge branch 'new-blueprint' into new-blueprint-updated.
382bb87
@pirmink
Updated reference for slow tests.
ca91e3c
Changed function, so it is covers the inversion of both coordinates i…
f4310b2 
…n lon (-180, 180) --> (180, -180)
Changed type hint comparison for the casting and Properties of the Te…
040ee5a 
…xtBoxAxes
@Karko93
Removed obsolete class in the Textbox Test
40c2288
@Karko93
Removed obsolete class in the textbox axes tests
125729d
@Karko93
Removed obsolete code
0c005b1
@Karko93
Removed DeprecationWarning coming from new matplotlib version due to …
797ed2a 
…changing in the colormapping
@Karko93
Updated the references in the slow test of the different models
7950291
@Karko93
Formatted Code
2eba1ae
@Karko93
Updated to pypdf because pyPDF2 is deprecated
c91ce0a
@Karko93
Updated environment and version information
103b0c7
@pirmink
Merge pull request #25 from Karko93/new-blueprint
3f3cc5c 
Updated pyflexplot to Python 3.10.12
@Karko93
Update Readme
01654a2
@Karko93
Merge pull request #26 from Karko93/environment-update
f49290d 
Update Readme
@clairemerker
Copy link

The pinned installation is not working for me. I tried this on balfrin tools/setup_env.sh -m
and got this error:

Could not solve for environment specs
Encountered problems while solving:
  - package libsqlite-3.43.0-h2797004_0 requires libzlib >=1.2.13,<1.3.0a0, but none of the providers can be installed

The environment can't be solved, aborting the operation

clairemerker
clairemerker suggested changes Oct 10, 2023
View reviewed changes
Copy link

@clairemerker clairemerker left a comment

Choose a reason for hiding this comment

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

I reviewed a few things related to the blueprint update, but it is probably not an extensive review. I hope it helps nonetheless!
Here are a few addtional general comments:

  • As mentioned, the pinned installation fails. Is this known behaviour and will be fixed later?
  • The unpinned installation worked, and the tests are all passing (with editable and non-editable installation)!
  • The pre-commit hooks fail. You might want to fix that before merging the PR.

environment.yml Outdated Show resolved Hide resolved
tests/slow/test_pyflexplot/test_plots/ref_cosmo-1_affected_area.py Show resolved Hide resolved
tools/find_py_toml.sh Outdated
Copy link

Choose a reason for hiding this comment

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

What is this script needed for? I can't find anything about it in the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just discovered this as well.
This tool requires config files in .toml format to define your output. There are also a lot of tests implemented and therefore, config files defines. This script prints a list of all .toml files within the src folder in order to find them faster I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall I just keep it?

Copy link

Choose a reason for hiding this comment

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

If you as a developer find it useful, then keep it of course.

Copy link

Choose a reason for hiding this comment

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

But maybe document it somewhere

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.meteoswiss.md Outdated Show resolved Hide resolved
@Karko93
Copy link
Contributor Author

Karko93 commented Oct 13, 2023

Regarding pre-commit hooks and pinned environment:
There is another PR in the pipeline in which a new feature was implemented.
On this particular branch I started to work on the pre-commit errors and the pinned environment is working and tested there.
I'm awaiting customers approval right now and then I'm going to make the pull request which will resolve these issues.

@Karko93 Karko93 merged commit c475c9d into main Oct 20, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clairemerker clairemerker clairemerker requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Karko93 @clairemerker @pirmink @twicki