Earth-Information-System / fireatlas

4 stars 2 forks source link

To Validate or Not To Validate EPSG Codes #98

Closed ranchodeluxe closed 1 month ago

ranchodeluxe commented 1 month ago

Problem

We currently validate which EPSG codes we allow folks to submit for a regional runs and analysis. There's a tradeoff here. If they do one not in the list they have to add it or the job will fail. But the benefit is if it's going to be a tricky projection from WGS84 to the expected CRS that makes them think about it

AC

zebbecker commented 1 month ago

@mccabete like you said, we may have a hard time providing descriptive warnings when EPSG errors are subtle. I feel like from what I've seen so far, the likely outcome is that the code will execute just fine, and we won't have an obvious place to throw an error. The "error" will be in the actual content of the output, which will have projection issues.

With regards to adding a dependency for checking the validity of reprojecting from one projection to another: I don't really want to do that, because my understanding is that we will only hit this issue once per new region. I feel like the decision about whether or not a new EPSG is appropriate should be made by a human who is knowledgable about the new region they are trying to add. Trying to import a library to do that for us seems like a lot of complexity for something that will only rarely come up, plus this strikes me as the sort of issue that may be difficult to encode a "right answer" for in a general purpose library. This impression is based on my limited understanding of the underlying geography stuff, though!

Anyways, what if we make the error more descriptive: something like "If you are really sure you want to be using this code, go add it to the approved codes here"? (this does contradict what I said about not wanting to have to change source code)

jsignell commented 1 month ago

I'm leaning very much towards don't validate. But maybe there is a step early on that is likely to throw an error if the crs doesn't project to WGS84 and we can try to wrap that error with a little bit of helpful information?

mccabete commented 1 month ago

Let's just throw a warning if it's not WGS84 and move on

mccabete commented 1 month ago

"If you are really sure you want to be using this code, go add it to the approved codes here"?

I essentially think that we should do this.

zebbecker commented 1 month ago

I'm a little unclear on consensus. As a starting point, PR 109 changes the validation so that if the EPSG code is not in our list of allowed codes, instead of throwing an error that stops everything, it just outputs a warning and continues on.

warnings.warn(f"EPSG projection code {epsg} not recognized as one of: {allowed}. (A new code can be registered in FireConsts.py if needed.)").

@mccabete am I interpreting correctly what we landed on for behavior?

@ranchodeluxe or @jsignell will warnings.warn play nicely with existing logging or should I be doing something else?

zebbecker commented 1 month ago

Thanks guys! Closed with 109