Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: None
-
Labels:None
-
Story Points:5
-
Epic Link:
-
Sprint:DRP S17-2, DRP S17-3
-
Team:Data Release Production
Description
In DM-9001, we tracked down José Garmilla's S/G separation code and archived it for future use.
On this ticket:
- Get the code working, making minor fixes as necessary;
- Ensure that it is adequately documented (in terms of README files, general explanations, etc), so that future developers can pick it up and understand how to use it.
At the end of this ticket, it should be possible to run the code and understand the outputs, but it's not necessary to perform any sort of integration with the LSST stack, or update it to match LSST coding standards, except insofar as that's necessary to get the basic functionality working.
Attachments
Issue Links
- blocks
-
- Done
Activity
analysis:
- The single commit here seems to consist mostly of whitespace changes. If these are strictly necessary, please separate them out and put them on a separate commit. As it is, they make this commit basically unreviewable (it took me a long time just to find any substantive changes to look at).
- What's the justification for this code being “effectively deprecated”? Do you just mean it doesn't work, or have you e.g. asked potential users, had an RFC, posted on CLO...?
sgs-fsbutler:
- A number of relatively minor cleanups requested on the PR.
- It would be nice if the commits here were more fine-grained (ie, each one addressing a separate issue, rather than all thrown together in one giant blob). Given the circumstances, you can probably get away with it, though.
sgs:
- More minor cleanups on the PR.
- Please make sure the commits are properly normalised. For example, c815e761f82d1d08b65554af2d0b325e60f83d81, which claims to just add a readme file, also seems to adjust .gitignore (maybe forgivable) and fromDbToTruth.py (definitely not).
Not yet had chance to look at sgsupervised; comments on that forthcoming.
sgsupervised:
- Minor comments on the PR.
- I've not carefully read all the changes here, because there's a lot of whitespace churn and formatting issues which makes it very hard to see what actually changed.
- It'd be really nice to have docstrings that cover what all these miscellaneous functions do and what the arguments mean. I don't think it's a requirement that you do that for every function within the scope of this ticket, but I do suggest that you make a brief note on which of the functions you've actually got working and what they do.
- In some places you've updated paths to be relative to sgsDir (I don't understand how you've defined that, but it's clearly the right thing to do). In other places, even in lines of code you've touched, there are still hard-coded paths referring to José's filesystem. I suspect this is just related to functions you've not tested (even though you reformatted them), in which case the point above applies. Please clarify.
Can you take a quick look at the changes made for the review and OK them?
Added a couple of trivial comments but basically this is fine, content wise.
However, before you merge please sort out the history. There are a bunch of weird commits in the ticket branch for sgs that clearly need to be rebased out. In fact, you can probably just squash the whole thing down to a single commit. Let me know if you need help with this.
History fixed and merged to master.
Hi John,
Could you please review the work done on this ticket? I've made changes to 4 repositories - they're linked in the usual place.