imSim

imSim copied to clipboard

@rmjarvis' comment today

One of my pet peeves is when people display anything about the night sky putting North up and East right. Which corresponds to the view from the CMB looking back through the universe at Earth. (!)

reminded me that in order to use the same convention as PhoSim we agreed to switch our convention on angles to be as above resulting in a minus sign in the lensing shear for DC2. This is different than at least what DES (and treecorr) uses. We should check the LSST project convention but I thought it was the same as DES.

This is a reminder to check the LSST convention so we don't forget about it, and likely to request that we switch this before DC3. It will get harder and harder to change in the future and will result in extra minus signs to remember.

I think (hope!) we finally removed the old PhoSim coordinate convention.

But, I thought we had some switches in the YAML to flip if really necessary. I don't see that now, so need to come back and check to document the current state before I close this.

@jchiang87 @rmjarvis I see a flip_g2 variable in the instcat code, but I don't see where to set it (I think this is left around from before actually). I don't see any other way to change it.

I don't see anything in the galsim config system or skycatalogs either. I want to get rid of the other convention entirely but I thought we had some discussion/code about a way to flip the value if it was necessary. Is that upstream somewhere else or in skyCatologs and we just assume we never have to deal with it now?

I'm trying to figure out where we stand and if we can close this now.

Hi All,

Pinging people again to look at this issue and see if it should be closed now.

Pinging people again on this.

So I can see that the parameter flip_g2 is set to True by default here:

https://github.com/LSSTDESC/imSim/blob/18b03e764cf396fbc1ecc7f35d3eed207a921c5e/imsim/instcat.py#L104

and is used to set the sign later. Is this what we really want if we want to use the Earth based coordinate system?

I think it depends on what the inputs are. For DC2 instance catalogs, I think we want flip_g2=True if we want to use the standard convention. However, someone can always produce an instance catalog that has the "correct" g2 sign and then this should be flip_g2=False, and ideally the user would be aware of this and set the config accordingly. There is access to the flip_g2 variable through the config here. It's optional, and by default it gets set to None, which behaves the same as flip_g2=False. Maybe that's the right default behavior? If not, then we need to fix how the default value is set via the config.

I thought it gets set to True here:

def __init__(self, file_name, wcs, xsize=4096, ysize=4096, sed_dir=None, edge_pix=100, sort_mag=True, flip_g2=True,
                 min_source=None, skip_invalid=True, logger=None):

which is why I was confused. Is it set to None in another way?

I think the convention in those instance catalog was the old PhoSim convention. So when reading an instance catalog, we want to switch it. The convention in the new SkyCatalogs is (I believe) correct, so we don't flip anything there.

Yeah, it's a bit confusing, but when the InstCat object is created by the InstCatLoader class, the InstCat.__init__(...) initializer is called with the kwargs set here, so that value of flip_g2=True is overridden by the value obtained from the config, which, as I noted, is set to None by default.

I don't think optional parameters should be getting set to None. They should be omitted from the kwargs. Are you sure it's getting set to None somehow?

So, I think I would like to not flip it for people writing new files and let people know if they want to use a previous DC2 catalog they should flip it. Hopefully, the DC2 stuff will fade away. So, I am happy if is is set to None by default, but I'm missing it somehow. Sorry!

Are you saying it is set if imsim-config.yaml ? I can't find it. The only place I see it is in the test_instcat_parser.py file.

I don't think optional parameters should be getting set to None. They should be omitted from the kwargs. Are you sure it's getting set to None somehow?

I can check, but we ran into this issue with something Geri was doing some months ago and it was being set to None. Maybe there was something else in that code that was causing the value to be None.

I added a line assert flip_g2 is not None, and all the unit tests pass. So if it is being set to None somewhere, it's not in a code path we exercise in the unit tests.

FWIW, I think we should keep the default =True for instance catalogs. I think it's unlikely that anyone will start making instance catalogs with the galsim sign convention. Pretty much all the instance catalogs we might expect people to use will be the old ones that do it the phosim way. And anyone who is still making new ones using some existing code either doesn't case (making point sources) or will most likely use the phosim convention, since that's what their code already uses. People who want to start from scratch to create a scene of objects IMO shouldn't use instance catalogs. There are better ways of defining various kinds of scenes that should be used instead.

Well, I want to erase all remnants of this convention as a default from our code. I objected strongly at the time we decided to keep this because changing it for PhoSim wasn't something that was considered feasible and predicted it was a mistake and we would still be dealing with it for years to come (I was right :))

So, I still want to remove it here if people are going to use this format at all. I think for people doing tests, trying something very small, and setting things up being able to simply write a text file without figuring out how to write something the sky catalog is going to be way better. Having a simple text file input is really important for non-expert who want to try things themselves and who want to make small scenes.

I would be OK with another text format and a convertor to put something into a format that we could also read (a binary format like Jim has suggested e,.g) or converting it in some other way into a sky catalog file that can be easily read would be OK,or if there was really an understandable easy way to do this as a single line int the config, but I am targeting non-large scale simulations here and I think it is important.

I just don't want to keep this convention around, and I don't think int instance catalogs are a pure legacy format yet.

Is there a pure straightforward config interface way that could be specified where they could specify all the parameters needed for each object including the g, dust parameters, etc on a line and have it read in? Or is that something we would need to add?

Having a simple text file input is really important for non-expert who want to try things themselves and who want to make small scenes.

There are more user-friendly ways to provide a text file than this.

GalSim demo 4 shows how to use an input catalog for all parameters. That can be either text or fits as preferred.

https://github.com/GalSim-developers/GalSim/blob/releases/2.4/examples/demo4.yaml

OK I will look at this and think about what a simple easily understandable replacement might look like:

In terms of template is this right?

We can do this:

template A
   template B
      my-file.yaml  (gets both A and B)

But we cannot do this:

template A
template B
    my-file.yaml  (gets both A and B)

So in other words: nested OK, calling more than one the template in the same file not OK.

Ugh.. my indentation that was supposed to make that clear got all ripped out :(

looks the same now.

OK updated... go back and look on the edited comment on GH itself.

The YAML file is a dict. So I don't know what it does with two identical keys, but it's either an error or the second one overwrites the first. In either case, once it converts to an actual dict in Python, there will only be one template item.

OK but does the 1st way work? Or are you saying both don't work?

I mean:

You call a template A file from Template B.

In file C you call template B.

In that case do you get both A and B? Or is there really only one dictionary and this won't work either?

Yes, the first way is fine. I was explaining why the second one cannot possibly work.

OK got it. Thanks.