CosmiQ / solaris

CosmiQ Works Geospatial Machine Learning Analysis Toolkit
https://solaris.readthedocs.io
Apache License 2.0
412 stars 112 forks source link

Add support for pathlib.Path objects as input #460

Closed remtav closed 1 year ago

remtav commented 2 years ago

Description

Add support for pathlib.Path objects

Fixes #459

Type of change

Please delete options that are not relevant.

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:

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

@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.

remtav commented 2 years ago

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

rbavery commented 2 years ago

@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 commented 1 year ago

@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!