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

New input format #5

Merged
merged 28 commits into from
Nov 15, 2022
Merged

New input format #5

merged 28 commits into from
Nov 15, 2022

Conversation

thibaulttabarin
Copy link
Member

Modifiy the Crop_imae_main.py

README.md

  • add example for metadata.json format
  • add command line instruction

Copy link
Member

@hlapp hlapp left a comment

Choose a reason for hiding this comment

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

See inline comment. I also assigned @johnbradley, as he'd understand the code and its history much better.

.github/workflows/Deploy_Crop_Image.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Crop_image_main.py Outdated Show resolved Hide resolved
@hlapp
Copy link
Member

hlapp commented Nov 10, 2022

@thibaulttabarin please do not simply "resolve" comments (what GitHub calls conversations) without an explanation or corresponding code change. Otherwise, you're essentially signaling that you're dismissing those comments, and without explanation, which was presumably not your intent.

thibaulttabarin and others added 7 commits November 10, 2022 09:09
typo fix

Co-authored-by: John Bradley <johnbradley2008@gmail.com>
Co-authored-by: John Bradley <johnbradley2008@gmail.com>
Co-authored-by: John Bradley <johnbradley2008@gmail.com>
Change the increase value to 10% in the example shown in the documentation
- The code int(abs()) force the bbox to be a list of positive integer is unnessacery in this case
- Some package framework using crop function require a list of integer but not crop from PIL
- The code int(abs()) was reminescence from habit to transfer the code to the right format
- Since this code added some confusion and it is currently unnessecary it can be remove
Replace the absolute link with relative link for better portability between branches
Copy link
Member

@hlapp hlapp left a comment

Choose a reason for hiding this comment

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

See inline suggestions and comments. I think there's one remaining issue, namely Metadata json file generated by [Drexel_metadata_rformatter] (https://github.com/hdr-bgnn/drexel_metadata_formatter) (BTW notice typo and extraneous space between ] and (). I think this needs to refer to the drexel_metadata repo as the one that's generating the metadata including the bounding box, followed perhaps by saying that this code needs the metadata to be reformatted as done by the drexel_metadata_reformatter code. Isn't that right?

Crop_image_main.py Outdated Show resolved Hide resolved
Crop_image_main.py Outdated Show resolved Hide resolved
Crop_image_main.py Outdated Show resolved Hide resolved
Crop_image_main.py Outdated Show resolved Hide resolved
Crop_image_main.py Outdated Show resolved Hide resolved
Crop_image_main.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
thibaulttabarin and others added 2 commits November 14, 2022 13:00
Co-authored-by: Hilmar Lapp <hlapp@drycafe.net>
Co-authored-by: Hilmar Lapp <hlapp@drycafe.net>
thibaulttabarin and others added 7 commits November 14, 2022 13:02
Co-authored-by: Hilmar Lapp <hlapp@drycafe.net>
Co-authored-by: Hilmar Lapp <hlapp@drycafe.net>
Co-authored-by: Hilmar Lapp <hlapp@drycafe.net>
Co-authored-by: Hilmar Lapp <hlapp@drycafe.net>
Co-authored-by: Hilmar Lapp <hlapp@drycafe.net>
Co-authored-by: Hilmar Lapp <hlapp@drycafe.net>
Copy link
Member

@hlapp hlapp left a comment

Choose a reason for hiding this comment

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

LGTM now.

@thibaulttabarin thibaulttabarin merged commit ecde661 into main Nov 15, 2022
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