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

read in the object ids from the instance catalogs as strings #151

Closed
jchiang87 opened this issue Sep 3, 2018 · 15 comments
Closed

read in the object ids from the instance catalogs as strings #151

jchiang87 opened this issue Sep 3, 2018 · 15 comments

Comments

@jchiang87
Copy link
Collaborator

Currently they are assumed to be ints. Looks like there are just two lines to change:
https://github.com/LSSTDESC/imSim/blob/master/python/desc/imsim/imSim.py#L186 and line 195.

phosim already allows for string ids in v3.7.x, and the SNe for DC2 may have string ids.

@jchiang87 jchiang87 added the DC2 label Sep 3, 2018
@jchiang87
Copy link
Collaborator Author

In sims_GalSimInterface, this line also explicitly expects the object ids to be an int:
https://github.com/lsst/sims_GalSimInterface/blob/master/python/lsst/sims/GalSimInterface/galSimInterpreter.py#L599

@cwwalter
Copy link
Member

cwwalter commented Sep 6, 2018

Recording information from CWW/JC in-person conversation.

This likely will have an effect on the RandomWalk galaxies and also could affect downstream programs as the code for the object type is encoded in the lower 10 bits of the integer uniqueID.

@cwwalter
Copy link
Member

cwwalter commented Sep 9, 2018

Is this still happening, or on hold/canceled due to the other ramifications of the change? Or still needs discussion with Rahul and all?

@jchiang87
Copy link
Collaborator Author

jchiang87 commented Sep 9, 2018

I think we need to make this change to support the WFD SNe. Those are the only object_ids that will not be cast-able as ints, so any downstream programs that read the instance catalogs will still be able to extract things like galaxy_id from the lower 10 bits.

@cwwalter
Copy link
Member

cwwalter commented Sep 9, 2018

OK. So it will always be an added sub-string to the original uniqueID (now as a string)? We already need this for 2.0i even without the sprinkler. Is that correct?

@EiffL just flagging you on this in case it is relevant to the knot class.

@jchiang87
Copy link
Collaborator Author

So it will always be an added sub-string to the original uniqueID (now as a string)?

I don't know the final format but it will be something like <galaxy_id>+<sn_id> = 1000001682_MS_564_6

We already need this for 2.0i even without the sprinkler. Is that correct?

Yes, this is for SNe in the WFD region which will be included even with the sprinkler disabled.

@EiffL
Copy link
Member

EiffL commented Sep 10, 2018

Argh, yes, changing the uniqueID will impact the RandomWalk (because the random seed used to sample the knots is based on the id), but that should be easy to accommodate. Just let me know if/when the change happens, and I can make sure the RandomWalk code gets properly updated

@cwwalter
Copy link
Member

Thanks Francois, don't let us close this issue unless it is working for you too...

@jchiang87
Copy link
Collaborator Author

@EiffL Regarding uniqueID, the only difference is that we'll store them as strings instead of ints, i.e., here
So if your knot objects continue to have ids that are cast-able as ints, the code that sets the random seed,
https://github.com/lsst/sims_GalSimInterface/blob/4c67ec1e7e6e55df837d5630ae9b75003d68c473/python/lsst/sims/GalSimInterface/galSimInterpreter.py#L454
should be fine.

@EiffL
Copy link
Member

EiffL commented Sep 11, 2018

agreed, it should be fine as long as the objectids cast to ints correctly

@cwwalter
Copy link
Member

Is there a PR for this one? Or is it in the commit of some of the others? I can't find it in imSim or sims_galSimInterface..

@jchiang87
Copy link
Collaborator Author

For imSim, it's integrated in the dc2_run2.0_rc branch directly (2786b1f), and there's this PR in sims_GalSimInterface

@cwwalter
Copy link
Member

OK got it; thanks. I had the centroid one, but I thought there were more. The Random knot changes are somewhere else?

@jchiang87
Copy link
Collaborator Author

The RandomWalk code isn't affected by this change since it had already cast the uniqueId to an int when passing it as a seed to the rng. As long as the instance catalog code doesn't use uncastable string ids for the knots, no changes are needed for the RandomWalk code.

@cwwalter
Copy link
Member

This change was made in the 2.0i development work.

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

No branches or pull requests

3 participants