spacetelescope / jwst_gtvt

Other
10 stars 11 forks source link

Add Backgroud Tool Functionality To GTVT #65

Open mfixstsci opened 3 years ago

pep8speaks commented 3 years ago

Hello @mfixstsci! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 27:29: E231 missing whitespace after ':' Line 27:39: E231 missing whitespace after ':' Line 31:64: E261 at least two spaces before inline comment Line 31:80: E501 line too long (92 > 79 characters) Line 56:80: E501 line too long (82 > 79 characters) Line 58:21: W291 trailing whitespace Line 64:80: E501 line too long (80 > 79 characters) Line 66:1: W293 blank line contains whitespace Line 97:80: E501 line too long (133 > 79 characters) Line 98:80: E501 line too long (135 > 79 characters) Line 124:1: W293 blank line contains whitespace Line 138:1: W293 blank line contains whitespace Line 139:78: W291 trailing whitespace Line 141:1: W293 blank line contains whitespace Line 146:1: W293 blank line contains whitespace Line 152:1: W293 blank line contains whitespace Line 155:1: W293 blank line contains whitespace Line 158:1: W293 blank line contains whitespace Line 162:1: W293 blank line contains whitespace Line 165:1: W293 blank line contains whitespace Line 168:1: W293 blank line contains whitespace Line 169:8: E741 ambiguous variable name 'I' Line 170:5: E741 ambiguous variable name 'I' Line 171:1: W293 blank line contains whitespace Line 173:1: W293 blank line contains whitespace Line 178:1: W293 blank line contains whitespace Line 180:1: W293 blank line contains whitespace Line 182:1: W293 blank line contains whitespace Line 184:1: W293 blank line contains whitespace Line 186:1: W293 blank line contains whitespace Line 188:1: W293 blank line contains whitespace Line 193:1: W293 blank line contains whitespace Line 200:1: W391 blank line at end of file

Line 27:80: E501 line too long (146 > 79 characters) Line 430:80: E501 line too long (83 > 79 characters) Line 431:80: E501 line too long (149 > 79 characters) Line 434:80: E501 line too long (98 > 79 characters) Line 438:80: E501 line too long (87 > 79 characters) Line 444:80: E501 line too long (93 > 79 characters) Line 452:80: E501 line too long (101 > 79 characters) Line 454:80: E501 line too long (140 > 79 characters) Line 457:51: E231 missing whitespace after ',' Line 458:80: E501 line too long (95 > 79 characters) Line 459:80: E501 line too long (111 > 79 characters) Line 462:51: E231 missing whitespace after ',' Line 463:80: E501 line too long (97 > 79 characters) Line 464:80: E501 line too long (111 > 79 characters) Line 467:51: E231 missing whitespace after ',' Line 468:80: E501 line too long (91 > 79 characters) Line 469:80: E501 line too long (111 > 79 characters) Line 472:51: E231 missing whitespace after ',' Line 473:80: E501 line too long (100 > 79 characters) Line 474:80: E501 line too long (111 > 79 characters) Line 477:51: E231 missing whitespace after ',' Line 478:80: E501 line too long (97 > 79 characters) Line 479:80: E501 line too long (111 > 79 characters) Line 482:51: E231 missing whitespace after ',' Line 483:80: E501 line too long (88 > 79 characters) Line 484:80: E501 line too long (111 > 79 characters) Line 492:80: E501 line too long (87 > 79 characters) Line 510:46: E231 missing whitespace after ',' Line 512:80: E501 line too long (89 > 79 characters) Line 513:80: E501 line too long (92 > 79 characters) Line 516:80: E501 line too long (206 > 79 characters) Line 517:80: E501 line too long (231 > 79 characters) Line 528:80: E501 line too long (122 > 79 characters) Line 542:46: E231 missing whitespace after ',' Line 545:80: E501 line too long (150 > 79 characters) Line 551:80: E501 line too long (122 > 79 characters) Line 893:1: E303 too many blank lines (3) Line 893:80: E501 line too long (113 > 79 characters) Line 919:80: E501 line too long (126 > 79 characters) Line 927:80: E501 line too long (105 > 79 characters) Line 975:80: E501 line too long (87 > 79 characters)

Comment last updated at 2021-10-22 19:40:06 UTC
mfixstsci commented 3 years ago

@bholler @JAStansberry here are some updates dealing with adding the backgrounds tool to the gtvt. I added a flag users can enter to set a background limit based on the results from the backgrounds tool.

$ jwst_gtvt 253.2458 2.4008 --no_verbose --bkg_cutoff 0.1

The red portion of the figure are available angles but have a background value >= 0.1 and the grey values are positions that have background angles <= 0.1.

Screen Shot 2021-10-06 at 1 53 03 PM

If users don't enter a background argument, you get the regular figure you expect.

bholler commented 3 years ago

@mfixstsci This looks really great! When I first saw it, my thought was "are the red regions good or bad?" Luckily there is a legend in the top right. I thought about suggesting making the viable regions green, but red right next to green is a nightmare for those with color blindness. The color selection and choice to color the bad regions is appropriate.

mfixstsci commented 3 years ago

@mfixstsci This looks really great! When I first saw it, my thought was "are the red regions good or bad?" Luckily there is a legend in the top right. I thought about suggesting making the viable regions green, but red right next to green is a nightmare for those with color blindness. The color selection and choice to color the bad regions is appropriate.

Hey @bholler glad to hear that! We should try and set up a meeting with the other stakeholders. I am working on updating the MTVT as well. You can call the MTVT with the back ground cut off flag as well but I am sure we are applying the back ground calculation from the last RA and DEC value of the moving target of our choosing. I haven't looked too into this, but plan to now that I got the GTVT and tests working correctly.

bholler commented 3 years ago

What exactly is the issue with the MTVT?

mfixstsci commented 3 years ago

What exactly is the issue with the MTVT?

@bholler, nothing is wrong with the MTVT, but I think we should take a little bit more care in how we apply the background tool implementation. Since the RA and DEC changes for the moving target over time, we want to keep track of that. Currently, you just pass one RA and DEC value for a fixed target and just monitor that over time, but with the MTVT, we will need to call the background tool every time the RA and DEC of our moving target changes.

What I was trying to say, is that the MTVT tool "works" with the background flag, but getting the values of the background changing as a function of RA and DEC is not done. If you call the MTVT jwst_mtvt Cere --bkg_cutoff 0.1 you will only get the background over the range of dates selected for the last RA and DEC of the moving target, but as I understand, this will be incorrect. Because the background for RA_1 and DEC_1 on DATE_A will be different for RA_2 and DEC_2 at DATE_A because they aren't in the same location. Maybe I am thinking too hard about this and hope this made a little bit more sense.

bholler commented 3 years ago

@mfixstsci This makes sense, thanks for the clarification. I agree the tool should take into account the movement of the target when computing the backgrounds, since the background at the beginning and end of a ~50 day window could differ significantly. If this takes an unreasonably long amount of time to run, then we would need to discuss mitigation strategies (e.g., determine the background for a 3-5 day window, etc.).

mfixstsci commented 3 years ago

Users can supply a num_cpu flag in the command line to speed up moving target support.

jwst_mtvt Ceres --bkg_cutoff 0.3 --num_cpu 16 --no_verbose

Screen Shot 2021-10-12 at 1 36 01 PM

Using all processors on my machine, it takes about 30 seconds. Only using 1 took about 4-5 minutes which is the default.

wblairstsci commented 3 years ago

First off, on GTVT, I like this change and just note that we need to update JDox appropriately when we officially announce the availability of the change. The JDox article might also need some clean up in terms of what we say about accessing the tool as well. (For instance, it currently talks about astroconda and pip install, etc. I would need some help there to make sure I say the right things.)

A question though about how the background is being calculated here: Is it calling the backgrounds tool in the background? Or is it just looking at a cached background database as a function of position and wavelength? Or what?

Besides knowing what to say in the documentation. this may also have application to the MTVT situation. If the background is being determined for tessellated regions on the sky, the small amount of movement of a moving target may not be an issues. (Only if the target moves across a boundary in the backgrounds file). Then again, I realize that comets in particular can move a LOT over a visibility window timeframe. So perhaps we do have to worry about it. (Maybe a flag for when the moving target is a comet would be helpful to determine when the full moving target strategy should be applied to the background calculation?) Just a thought.

wblairstsci commented 3 years ago

Correction: IN reviewing the description of the JBT itself, indeed it uses the pre-computed background cache! (I had forgotten the details.) So in any event, factor this into the discussion above. Sorry for the confusion.

bholler commented 3 years ago

@wblairstsci I would be happy to contribute to the GTVT page edits in JDOX; I believe I have done so in the past, specifically for changes to installation instructions. We will have to remove all mention of astroconda since it is no longer supported.

As to the question about the MTVT, I'm not sure there would be a simple way to determine when the target crossed a boundary in the background cache. Such a thing would require a pretty complex look-up table since I don't imagine the boundaries exactly follow lines of RA and DEC. Creating said look-up table would also be a large amount of work. So calling the JBT at every step is probably the best way to go. I will be sure to add a caveat to the JDOX page that running the JBT with the MTVT can take a while! The strategy here would be to shrink the date range down to dates of interest; this significantly speeds up the MTVT + JBT runs.

mfixstsci commented 3 years ago

@bholler and @wblairstsci I will admit the solution I implemented for the mtvt is pretty heavy handed in terms of just calling the jbt every single time you need to compute the background when the RA and DEC changes slightly. We could potentially look at how much the background changes as a function of RA and DEC and apply a more clever solution. For instance, if the background is only calculated for whole number RA and DEC values in degrees and the mtvt is returning RA and DEC to some decimal precision, maybe we only trigger calls to jbt when the RA or DEC increases by a full degree. I like the idea of selecting a date range as well, should we investigate these idea solutions before merging?

wblairstsci commented 3 years ago

@mfixstsci and @bholler : I had a conversation with Bryan yesterday. I agree it is overkill. Basically, all moving targets are not created equal. For slow moving targets, the background calculation will really not change significantly. This is because the cached background file that the JBT uses uses some sort of tessellated sky pattern of background positions and motions within each patch will not change the background. It would only change if the moving target moves across one of these boundaries, and even if it does, the background does not change dramatically from one sky region to the neighboring one. So in principle, a fast moving target has a better chance of crossing a boundary, but even then, it may not matter that much! (I have a question out to try and understand what the size of the sky regions is and whether it is really a constant size or whatever. That might inform how often we would even expect a MT to potentially cross a boundary in the background cache file.)

The question is, what to do about it in the tool? Options I see are: a) brute force, calculate at every position (even though it is overkill); b) assess the total motion of the MT over a given visibility period and just do the background calc for a position at the mid-point of the track; or c) develop a more complicated model that factors the actual motions and background model details (like sky region boundaries) into the assessment.

Option c is way beyond the boundary of value added, in my opinion. (After all, recall that the GTVT and MTVT are front end "quick look" tools!) Option a is apparently the current default. So I guess is it worth considering something along the lines of option b??

mfixstsci commented 3 years ago

@wblairstsci & @bholler I see the three options and agree. I have already implemented option A which isn't ideal but if we just tell users it might take a couple of seconds, that might be the deal we have to make. There is a progress bar and an option for users to speed up the compute, are there any caveats or notes we should make about moving target/background support? How accurate it is?

Also, units, do you know the units the background is calculated in? Is it a percentage? I just feel obligated to add the units to the legend/docs for the command line inputs etc

wblairstsci commented 3 years ago

Backgrounds are in units of MegaJanskys per steradian, abbreviated MJy/sr. It is definitely not a percentage or percentile-type measurement!