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

Upgrades #12

Merged
merged 6 commits into from
Jan 15, 2020
Merged

Upgrades #12

merged 6 commits into from
Jan 15, 2020

Conversation

rbeyer
Copy link
Member

@rbeyer rbeyer commented Jan 7, 2020

Made a bunch of additions as I was working on my latest cycle.

@guymac
Copy link

guymac commented Jan 7, 2020

As a (really far out) aside, it turns out that hyperlinks in MS Office documents won't work for any site that does authentication and uses a cookie to remember your session ID between requests; Office does the request internally, then uses the URL from that request (typically the login page) as the one that is passed to the browser. They do work as expected in Google Sheets for instance. ;-)

@rbeyer
Copy link
Member Author

rbeyer commented Jan 7, 2020

So are you saying it shouldn't work from Excel, because it 'works as expected' as far as I can tell.

I did a test with Excel (I mostly use LibreOffice for my HiTList work), and it loads up the formula just fine. Sure, on the first try you land at the HiReport log-in page, but once you log in, you get to the page you clicked on. After that, when I click on links in those cells in Excel, I get taken straight to the HiReport page, so it works as I expect it to (I have to log in once, and then I'm good to go).

@guymac
Copy link

guymac commented Jan 7, 2020

What version/OS of Excel? I ran into this problem (with HiReport links in a .xlsx) last year using the latest Excel on Windows 10. https://stackoverflow.com/questions/2653626/why-are-cookies-unrecognized-when-a-link-is-clicked-from-an-external-source-i-e

@rbeyer
Copy link
Member Author

rbeyer commented Jan 7, 2020

I only tested on my macOS Mojave 10.14.6 system with Excel version 16.32.

@SchallerSpace
Copy link
Member

Some comments here based on my understanding of the uplink ops engineer process and background

  1. CSV_to_PTF is now included in the HiPlan distribution, but it does ignore columns past the 31 PTF columns, so the option in ptf2csv.py to append a spreadsheet link shouldn't interfere. We'll want to check that of course, but I really don't think it'll be an issue.

  2. Without having yet looked at the code for prioritize_by_orbit.py and priority_rewrite.py, I'm not sure if the priority adjustments will match the specific ranges of priorities in use. I'm guessing they do.

  3. The README.rst file lines for prioritize_by_orbit.py refer to a "shadow" zone ("'shadowed' by latitude-exclusion zone") that might cause some double-takes and misunderstanding. "Shadow zone" as a term is widely used within internal ops to refer specifically to the longitudinal zone imposed by ELECTRA relay operations around the Curiosity rover and, one expects, the Dusty rover (Mars 2020). It might be good to avoid overloading this term here.

@SchallerSpace
Copy link
Member

Hm. The PTF SIS notes the request priority field is an integer, saying nothing about sign. MTT allows negative priorities as well (as per the SIS). It's never occurred to me that we might have negative priorities, though, and I wonder if there are any tools out there in the wild, HiROC or JPL, that assume it's never negative. (prioritize_by_orbit.py here.)

@SchallerSpace
Copy link
Member

I should really preserve these for one big comment but why start now? I wonder if a preference for preserving observations closer to the equator is the best or only strategy. Maybe the closer to the start of the dayside pass (or closer to the end of it) might be another preference?

@SchallerSpace
Copy link
Member

The priorities as HiSEAS defines and uses them. I don't know if this is a complete list; it certainly doesn't include non-stereos.

     4000      : unprioritized SPORC
    10000-10015: SPORC stereo-1
    11000      : want-to-have SPORC stereo-1
    13500-14500: stereo-2
    14600-14615: SPORC stereo-2
    14700      : want-to-have stereo-2 

Copy link
Member

@SchallerSpace SchallerSpace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some notes in the pull request thread; I don't think anything here is a problem, just comments on items that could prove to be minor irritants.

@rbeyer
Copy link
Member Author

rbeyer commented Jan 14, 2020

  1. Without having yet looked at the code for prioritize_by_orbit.py and priority_rewrite.py, I'm not sure if the priority adjustments will match the specific ranges of priorities in use. I'm guessing they do.

I'm assuming that by 'specific ranges of priorities in use' you are referring to the common default priority values that suggestions are assigned when a HiTList is given to a CIPP, as you define in your HiSEAS comment. Doesn't matter to these tools for these reasons:

prioritize_by_orbit.py does not change the absolute value of any priorities, it only multiplies some by negative one, if it doesn't think it can fit on an orbit.

priority_rewrite.py can change the priority value, but its application is meant to mimic what a CIPP does anyway in order to give 'unique' priorities for each suggestion in the HiTList, and is completely configurable.

All of that being said, you could change the 'usual prioritiy scheme in HiSEAS' and still use these tools just fine.

  1. The README.rst file lines for prioritize_by_orbit.py refer to a "shadow" zone ("'shadowed' by latitude-exclusion zone") that might cause some double-takes and misunderstanding. "Shadow zone" as a term is widely used within internal ops to refer specifically to the longitudinal zone imposed by ELECTRA relay operations around the Curiosity rover and, one expects, the Dusty rover (Mars 2020). It might be good to avoid overloading this term here.

Wasn't aware of that usage. If I just call it a 'latitude-exclusion zone' do you think that would be okay?

@rbeyer
Copy link
Member Author

rbeyer commented Jan 14, 2020

I should really preserve these for one big comment but why start now? I wonder if a preference for preserving observations closer to the equator is the best or only strategy. Maybe the closer to the start of the dayside pass (or closer to the end of it) might be another preference?

Could be. If someone felt that was important, it could be integrated and provided as an option.

@rbeyer
Copy link
Member Author

rbeyer commented Jan 14, 2020

Hm. The PTF SIS notes the request priority field is an integer, saying nothing about sign. MTT allows negative priorities as well (as per the SIS). It's never occurred to me that we might have negative priorities, though, and I wonder if there are any tools out there in the wild, HiROC or JPL, that assume it's never negative. (prioritize_by_orbit.py here.)

Yeah, I had never seen any negative priorities, and I was quite excited when I lit on the idea of flipping the sign of a priority, as that allowed me an easy way to indicate that an observation was 'out' or 'deprioritized' without having to delete it, or mangle its original priority value. This is valuable while prioritizing the HiTList, because you sometimes find yourself doubling back on an orbit or set of observations and reevaluating, and just being able to flip the sign made that work much smoother than previous attempts. Furthermore, after you've done your work, and have a 'final' HiTList, and are ready priority_rewrite.py to do its work, you can have it remove all the suggestions with a negative priority, leaving you with a lean, clean HiTList to return to your HiTS.

@SchallerSpace
Copy link
Member

SchallerSpace commented Jan 14, 2020

  1. Without having yet looked at the code for prioritize_by_orbit.py and priority_rewrite.py, I'm not sure if the priority adjustments will match the specific ranges of priorities in use. I'm guessing they do.

I'm assuming that by 'specific ranges of priorities in use' you are referring to the common default priority values that suggestions are assigned when a HiTList is given to a CIPP, as you define in your HiSEAS comment. Doesn't matter to these tools for these reasons:

Okay, cool - and I've checked with uplink, looks like that set of HiSEAS ranges is the only one we currently treat in any special way.

  1. The README.rst file lines for prioritize_by_orbit.py refer to a "shadow" zone ("'shadowed' by latitude-exclusion zone") that might cause some double-takes and misunderstanding. "Shadow zone" as a term is widely used within internal ops to refer specifically to the longitudinal zone imposed by ELECTRA relay operations around the Curiosity rover and, one expects, the Dusty rover (Mars 2020). It might be good to avoid overloading this term here.

Wasn't aware of that usage. If I just call it a 'latitude-exclusion zone' do you think that would be okay?

Ya I think that'd be great if it's not burdensome. We've got exclusion zones in ops as well, of course, but "shadow zone" takes on its own meaning with its own set of frustrations. The ELECTRA shadow zones are causing problems for CaSSIS, too, because TGO has an ELECTRA and they are extremely conservative with how they use ELECTRA. ("They" being the ELE team, I believe, not necessarily ESA.)

@SchallerSpace
Copy link
Member

SchallerSpace commented Jan 14, 2020

Hm. The PTF SIS notes the request priority field is an integer, saying nothing about sign. MTT allows negative priorities as well (as per the SIS). It's never occurred to me that we might have negative priorities, though, and I wonder if there are any tools out there in the wild, HiROC or JPL, that assume it's never negative. (prioritize_by_orbit.py here.)

Yeah, I had never seen any negative priorities, and I was quite excited when I lit on the idea of flipping the sign of a priority, as that allowed me an easy way to indicate that an observation was 'out' or 'deprioritized' without having to delete it, or mangle its original priority value. This is valuable while prioritizing the HiTList, because you sometimes find yourself doubling back on an orbit or set of observations and reevaluating, and just being able to flip the sign made that work much smoother than previous attempts. Furthermore, after you've done your work, and have a 'final' HiTList, and are ready priority_rewrite.py to do its work, you can have it remove all the suggestions with a negative priority, leaving you with a lean, clean HiTList to return to your HiTS.

Right, that's very cool. After discussions with Nicole FB this morning (she also didn't realize negatives are allowed), I think we'll probably add a check in HiPolish to make sure there aren't any negative values in there. I don't think any of the HiRISE software will fail because of a negative req. priority, but we're less certain about JPL's tools.

I think you'd have to go out of your way to get hung up on a negative integer in Perl (which… what are integers anyway? what are data types? what is anything but a list? there are naught but lists in the void), and maybe a little less out of your way in Java, but still, we can be cautious on our end and as you note, wow, what a great way to use it for the CIPPs!

Edit: Even though priority_rewrite.py will clean out the negatives. This is just an abundance of caution on our end, because sometimes software can surprise us… No action here; simple action for HiOps.

@rbeyer
Copy link
Member Author

rbeyer commented Jan 14, 2020

Well, sorry to cause a bunch of work on your end. Not my intent. However, if you only want to allow positive integers in that field, then at some stage the tools really should be making sure that you don't get floats, strings, and yes, negative integers, in there. And probably not at just one stage, especially if there's expected to be lots of changes to values in that field.

@SchallerSpace
Copy link
Member

Well, sorry to cause a bunch of work on your end. Not my intent. However, if you only want to allow positive integers in that field, then at some stage the tools really should be making sure that you don't get floats, strings, and yes, negative integers, in there. And probably not at just one stage, especially if there's expected to be lots of changes to values in that field.

Oh no no, it's not extra work, or not a bunch of work, and not untoward or a problem. MTT does check for data type (Java being strictly typed). I think we're just experiencing surprise that this is allowed by the SIS coupled with the knowledge that we don't really know what JPL's other tools will do and the fact that they don't have much of a budget for MRO software updates…

Anyway, this isn't a problem. :)

@rbeyer
Copy link
Member Author

rbeyer commented Jan 15, 2020

There's one approval here, what's the rule for merging? #11 wasn't answered.

@SchallerSpace
Copy link
Member

Thanks, Ross. I'll go ahead and merge. One reviewer should be sufficient for this project; we only use one internally for HiPlan, etc., for instance. Clearly we have some more thinking to do here.

@SchallerSpace SchallerSpace merged commit 2e54df8 into master Jan 15, 2020
@SchallerSpace SchallerSpace deleted the upgrades branch January 15, 2020 23:47
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

Successfully merging this pull request may close these issues.

3 participants