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

Add support for pathlib.Path objects as input #460

Closed
wants to merge 11 commits into from
Closed

Add support for pathlib.Path objects as input #460

wants to merge 11 commits into from

Conversation

remtav
Copy link

@remtav remtav commented Jun 6, 2022 •
edited

Description

Add support for pathlib.Path objects

Fixes #459

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected - these changes will not be merged until major releases!)

How Has This Been Tested?

Please describe tests that you added to the pytest codebase (if applicable).

3 very simple tests have been added to test the core input reading function for vector and raster files. These tests make sure a pathlib.Path object pointing to a file are succesfully read by the different utilities.

Checklist:

  • My PR has a descriptive title
  • My code follows PEP8
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new errors
  • I have added tests that prove my fix is effective or that my feature works
  • My PR passes Travis CI tests
  • My PR does not reduce coverage in Codecov

If your PR does not fulfill all of the requirements in the checklist above, that's OK! Just prepend [WIP] to the PR title until they are all satisfied. If you need help, @-mention a maintainer and/or add the Status: Help Needed label.

@remtav remtav changed the base branch from main to 0.5.0 June 6, 2022 20:48
@remtav
base.py:
a3ddd71 
- refactor Evaluator's __init__ to better use existing utilities
- load_truth() uses dedicated _check_gdf_load() function
core.py: _check_gdf_load() returns empty gdf with sindex if error raised
@remtav
test_core.py: add tests from pathlib.Path objects (necessary?)
1356c8d
@remtav remtav changed the title [WIP] Add support for pathlib.Path objects as input Add support for pathlib.Path objects as input Jun 8, 2022
@remtav
Copy link
Author

remtav commented Jun 8, 2022

@dphogan This PR would be ready for review. I don't think I can launch CI tests myself. Am I wrong?
Also, I added unit tests for Path object inputs, but I don't think they are essential. I can remove if not necessary.

@remtav
Copy link
Author

remtav commented Jun 16, 2022

@dphogan This PR would be ready for review. I don't think I can launch CI tests myself. Am I wrong? Also, I added unit tests for Path object inputs, but I don't think they are essential. I can remove if not necessary.

I must've pinged the wrond contributor...
@rbavery @srmsoumya ?

@rbavery
Update environment.yaml
aebd3ed
@rbavery
Merge pull request CosmiQ#464 from CosmiQ/fix-environment-install-by-…
42d5252 
…updating-opencv-name-in-yml

fix environment install, rename opencv-python > opencv
@rbavery
Copy link
Contributor

rbavery commented Jun 30, 2022

Hi @remtav you caught me in a busy month while I've been on vacation or absorbed with project work. I appreciate this PR, looks useful. I'll review it and the policy for launching CI tests next week.

@remtav
Copy link
Author

remtav commented Jul 6, 2022

@rbavery have you a chance to take a look? I'll be leaving next monday until mi-august, was trying to close up a few things before leave :)

@rbavery
Copy link
Contributor

rbavery commented Jul 6, 2022

@remtav I have not, but it looks like for some reason I don't even have an option to run the CI against your first-time PR. Once I can confirm tests pass this can be merged. This might wait until later this week.

@rodrigoalmeida94 any insight on what I'd need to change to allow CI to run on this PR? looks like in tests.yaml it's set to run on push, which I assume applies to all branches. Since @remtav is a first-time contributor he needs approval I think, but I don't think we are seeing the option we need to approve CI. https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

@rodrigoalmeida94
Copy link
Collaborator

@rbavery I think if you add on: [push, pull_request] instead of just on: push this would allow you to run the workflows against PRs from public forks.

@rbavery
add pull request trigger
a8b73a2 
attempting to allow first time contributors to run CI on their PRs
@remtav
Copy link
Author

remtav commented Aug 23, 2022

@rbavery friendly reminder about this PR and allowing CI tests for external contributions. Keep us posted.

@rbavery
Merge pull request CosmiQ#466 from CosmiQ/add-pr-trigger
d05ea3e 
add pull request trigger for first time contributors to run ci cd when PRing from a fork
@rbavery
Copy link
Contributor

rbavery commented Sep 7, 2022

@remtav With new PRs, CI/CD should run. Can you open a new PR that is rebased on https://github.com/CosmiQ/solaris/tree/0.5.0 with your changes?

@remtav
add support for pathlib.Path object as input to all functions accepti…
dfaa528 
…ng paths as strings
@remtav
base.py:
31c5b7e 
- refactor Evaluator's __init__ to better use existing utilities
- load_truth() uses dedicated _check_gdf_load() function
core.py: _check_gdf_load() returns empty gdf with sindex if error raised
@remtav
test_core.py: add tests from pathlib.Path objects (necessary?)
1d0f6f7
@remtav
Merge remote-tracking branch 'origin/459-pathlib' into 459-pathlib
41c4500
@remtav
Copy link
Author

remtav commented Oct 20, 2022

@remtav With new PRs, CI/CD should run. Can you open a new PR that is rebased on https://github.com/CosmiQ/solaris/tree/0.5.0 with your changes?

Ok!

@remtav remtav closed this Oct 20, 2022
@remtav remtav mentioned this pull request Oct 20, 2022
Open
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE]: Add support for pathlib.Path objects in raster and vector modules
3 participants
@remtav @rbavery @rodrigoalmeida94