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

updating Vac mvm (visual_morphology script) #736

Merged
merged 2 commits into from
Aug 27, 2021

Conversation

jvazquez77
Copy link
Contributor

Fixes #

This pull request:

  • Has a title that summarises what is changing.
  • Updates the documentation accordingly.
  • Has unit tests & code coverage is not adversely affected (within reason).
  • Works with Python 2.7 and 3.6 (and ideally with Python 3.7).
  • Updates the CHANGELOG.
  • Removes more lines of code than it adds.
  • If relevant, adds a new entry to the What's new? page.

@havok2063 havok2063 changed the base branch from master to vacs_dr17 July 15, 2021 14:35
Copy link
Collaborator

@havok2063 havok2063 left a comment

Choose a reason for hiding this comment

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

@jvazquez77 Since Marvin will support both DR16 and DR17 public data releases, your VAC will need to support both versions as well. I think you'll need to keep the old logic in some form to sort out the survey images from DR16, but modify the code to also work with the simpler DR17 images. Let me know if you want to iterate over ideas.

@jvazquez77
Copy link
Contributor Author

Hi @havok2063
Thanks for your comment. In the new set of images (DR17), we have combined the most important information from the 2 sets in DR16, and also we have corrected different errors. In this sense we think it is much better just to present the last version of images. On the other hand, please let me know if you consider Marvin must keep available both versions, I am happy modify the code and iterate over ideas.

@havok2063
Copy link
Collaborator

Hi @jvazquez77, yeah I think since Marvin will be backwards compatible with DR16 I think your VAC needs to be as well. Some users might access targets for DR16, and if they access your VAC for that target, we want the VAC to work as expected with the old data. The old data will still exist on the SAS and users can in principle use it. I agree that we want users to use the updated DR17 dataset. In that case, one idea could be to issue a warning when users are accessing your DR16 VAC data, to instruct them to try the updated data. E.g. in the get_target method, somewhere in the beginning, you can add something like

if parent_object.release == 'DR16':
    log.warning('You are accessing outdated DR16 data for this VAC.  This target has updated data in DR17.  We recommend using the new data release instead.')

or whatever message you would like to say. You would need to add a from marvin import log at the top.

@jvazquez77
Copy link
Contributor Author

@havok2063 I did the proposed changes, so I think now it is working fine, either for the DR16 or the DR17 version.
Please let me know if there is any comment.

@havok2063 havok2063 merged commit 3ac2637 into sdss:vacs_dr17 Aug 27, 2021
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.

2 participants