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

use forced_phot dependency instead of copied code #408

Merged
merged 5 commits into from
Nov 24, 2020
Merged

Conversation

marxide
Copy link
Contributor

@marxide marxide commented Nov 4, 2020

Replace the copied version of forced_phot with a pip depdendency to the forced_phot repo.
Closes askap-vast/forced_phot#4.

Replace the copied version of forced_phot with a pip depdendency to the forced_phot repo.
Closes askap-vast/forced_phot#4.
@github-actions github-actions bot added this to In progress in Pipeline Backlog Nov 4, 2020
@marxide marxide requested review from srggrs and ajstewart and removed request for srggrs November 4, 2020 19:49
@marxide marxide self-assigned this Nov 4, 2020
Copy link
Contributor

@ajstewart ajstewart left a comment

Choose a reason for hiding this comment

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

The pipeline changes look good, but on the forced photometry side I agree with the steps that @srggrs said:

  • move the code out of __init__.py and into forced_phot.py
  • make a tag and a release
  • set up the git submodule in Vast pipeline with reference to a specific tag

The last one has been done, it would be good to do the first two as well.

Pipeline Backlog automation moved this from In progress to Review in progress Nov 4, 2020
@@ -12,6 +12,7 @@ django-q==1.3.3
django-tagulous @ git+https://github.com/marxide/django-tagulous.git@65cbbf703c4af9c1287681788a27217535c44921
djangorestframework-datatables==0.5.1
djangorestframework==3.11.0
forced-phot @ git+https://github.com/askap-vast/forced_phot.git@v0.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

is that repo public?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess while this repo is also still private that one also doesn't need to be. We should make sure both become public when the pipeline is made public however, maybe create a specific issue so we don't forget.

Copy link
Contributor

@ajstewart ajstewart left a comment

Choose a reason for hiding this comment

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

Looks good to me, can you confirm that you've been able to test successfully, including seeing that the out of range filtering working.

I've opened #414 to address @srggrs point about the repos being public.

Pipeline Backlog automation moved this from Review in progress to Reviewer approved Nov 18, 2020
srggrs
srggrs previously approved these changes Nov 18, 2020
@marxide
Copy link
Contributor Author

marxide commented Nov 23, 2020

Looks good to me, can you confirm that you've been able to test successfully, including seeing that the out of range filtering working.

I've opened #414 to address @srggrs point about the repos being public.

I don't think the tests I run cover that specific case. I see this when I run both the master version and this version with debug logging on:

forced_phot DEBUG Removing 0 sources that are outside of the image range

The forced_measurement parquet files are also identical, except for the component names that are based on a timestamp.

Pipeline Backlog automation moved this from Reviewer approved to Review in progress Nov 24, 2020
ajstewart
ajstewart previously approved these changes Nov 24, 2020
Copy link
Contributor

@ajstewart ajstewart left a comment

Choose a reason for hiding this comment

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

I've tested this myself and it is working ok. Good to go.

Pipeline Backlog automation moved this from Review in progress to Reviewer approved Nov 24, 2020
Pipeline Backlog automation moved this from Reviewer approved to Review in progress Nov 24, 2020
@marxide marxide merged commit d33edad into master Nov 24, 2020
Pipeline Backlog automation moved this from Review in progress to Done Nov 24, 2020
@marxide marxide deleted the forced_phot-dep branch November 24, 2020 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Migration of the code base
3 participants