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

Add support for pathlib for reading PEtab tables #93

Merged
merged 5 commits into from
Nov 29, 2021
Merged

Conversation

dweindl
Copy link
Member

@dweindl dweindl commented Nov 29, 2021

Closes #82

@dweindl dweindl requested a review from dilpath November 29, 2021 14:09
@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2021

Codecov Report

Merging #93 (f1c709d) into develop (4f6b841) will increase coverage by 0.04%.
The diff coverage is 90.56%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #93      +/-   ##
===========================================
+ Coverage    79.65%   79.70%   +0.04%     
===========================================
  Files           26       26              
  Lines         2935     2941       +6     
  Branches       709      709              
===========================================
+ Hits          2338     2344       +6     
  Misses         422      422              
  Partials       175      175              
Impacted Files Coverage Δ
petab/problem.py 64.47% <33.33%> (ø)
petab/yaml.py 90.78% <92.30%> (ø)
petab/conditions.py 90.69% <100.00%> (+0.22%) ⬆️
petab/core.py 87.02% <100.00%> (+0.09%) ⬆️
petab/measurements.py 82.02% <100.00%> (+0.20%) ⬆️
petab/observables.py 97.14% <100.00%> (+0.04%) ⬆️
petab/parameters.py 86.53% <100.00%> (+0.08%) ⬆️
petab/sbml.py 57.97% <100.00%> (+0.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f6b841...f1c709d. Read the comment docs.

Copy link
Member

@dilpath dilpath left a comment

Choose a reason for hiding this comment

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

👍

Maybe nice to define e.g. petab.C.TYPE_PATH = Union[str, Path], could make the type hints a little neater. Not sure if it can be used as isinstance(..., TYPE_PATH) though.

petab/yaml.py Outdated Show resolved Hide resolved
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
@dweindl
Copy link
Member Author

dweindl commented Nov 29, 2021

Maybe nice to define e.g. petab.C.TYPE_PATH = Union[str, Path], could make the type hints a little neater. Not sure if it can be used as isinstance(..., TYPE_PATH) though.

Does not look very neat, agreed. Briefly thought about it, but then decided that it might be more convenient to users to directly have the types there, without looking up what the other type is...

@dilpath
Copy link
Member

dilpath commented Nov 29, 2021

Ah, additionally, the tests were changed to use Path, so maybe str is no longer tested? Fine as is though

@dweindl dweindl merged commit 7a2ac6e into develop Nov 29, 2021
@dweindl dweindl deleted the feature_pathlib branch November 29, 2021 14:52
dweindl added a commit that referenced this pull request Nov 29, 2021
* Run test suite on windows-latest, macos-latest, ubuntu-latest
* Fixed various issues related to temporary files on Windows
* Add support for pathlib.Path to some functions missed in #93 
* Fix loading remote yaml files on Windows

Closes #80

Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
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