8000 Fix oba msids by matthewdahmer · Pull Request #115 · sot/cheta · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix oba msids #115

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 11 commits into from
Closed

Fix oba msids #115

wants to merge 11 commits into from

Conversation

matthewdahmer
Copy link
Contributor

@taldcroft
@jeanconn

This update includes a fix for the 4OAVOBAT telescope msid which continues to show the narrow range version of this msid where it should be reporting the wide range version after the associated patch was uplinked. This update also includes a minor fix for the DP_OBA_AVE derived parameter.

@taldcroft
Copy link
Member

Try to do this without repeating code. Hint, with good surrounding comments you can hijack the msid_nums dict a bit:

'b': '30 31 33 34 35 36 37 38 39 40 41 44 45 46 49 50 51 52 53 54 4OAVOBAT'.split()

Then you just need to handle the oddball in the loop.

@matthewdahmer
Copy link
Contributor Author

Ok that works too. I wanted to avoid messing with the logic in the loop, but the result does look cleaner.

taldcroft
taldcroft reviewed Jan 7, 2016
View reviewed changes
Ska/engarchive/converters.py
msid_wide = '4OAVOBAT_WIDE'
else:
msid = 'OOBTHR' + msid_num
msid_wide = msid + '_WIDE'
Copy link
Member

Choose a reason for hiding this comment

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

You can pull this line out of the else: and remove line 371 above.

@matthewdahmer
Copy link
Contributor Author

True, that eliminates another line, new commit to follow momentarily.

@taldcroft
Copy link
Member

@matthewdahmer - I rebased this on the state-bins branch prior to regenerating data.

@matthewdahmer
Copy link
Contributor Author

@taldcroft - Thank You!
I take it I will not need to manually update this data then correct?

@taldcroft
Copy link
Member

@matthewdahmer - there is a slight problem with the 4OAVOBAT patch, which is that this MSID occurs in the TEL2ENG data content, not OBC4ENG like the others. So I would suggest that you back out the changes in def obc4eng(..) and make a new function that is fairly similar but is named def tel2eng() and fixes 4OAVOBAT. Does 4OAVHRMT need to have the same patch, or was that not changed on board? In the def tel2eng() function you probably don't need all the a/b logic since there was just one patch time. Sorry for not noticing this earlier.

@taldcroft
Copy link
Member

BTW, because I have rebased this onto the other branch, you need to get into your git repo and do something like:

git branch -m fix_oba_msids fix_oba_msids_bak
git fetch origin
git checkout fix_oba_msids
<edit as normal>
git push origin fix_oba_msids

Without moving the original branch out of the way there will be a merge conflict later when you try to push back up.

@matthewdahmer
Copy link
Contributor Author

That should do it. I should have noticed that I was using the wrong data content directory. 4OAVHRMT is not affected as the constituent HRMA zones were not switched to wide range. Please let me know if you have any comments or suggestions.

@taldcroft
Copy link
Member

OK thanks, looks good.

@taldcroft taldcroft mentioned this pull request Jun 13, 2016
Merged
@taldcroft
Copy link
Member

Relevant commits included (in squashed form) in #127. Closing.

@taldcroft taldcroft closed this Jun 14, 2016
@taldcroft taldcroft deleted the fix_oba_msids branch December 28, 2016 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@matthewdahmer @taldcroft
0