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

Remove update range #417

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

DougBurke
Copy link
Member

Remove the update_column_range script and code to use it when reprojecting and merging event files. The functionality has not been needed since CIAO 4.6 (if not earlier).

One minor difference is that update_column_range could clip the TLMIN/MAX field to a .5 value, whereas we now just get the actual limit.

The runtool module has been updated to remove update_column_range and it has also picked up some other recent changes, both to the contrib code (eg #396) and CIAO

reproject_events has long-since been updated to update the TLMIN/MAX
values for the SKY column when reprojecting data, and dmmerge seems
to do the correct thing too.
This has also picked up a few other updates to the runtool
module.
@DougBurke DougBurke linked an issue Nov 4, 2020 that may be closed by this pull request
@DougBurke
Copy link
Member Author

@kjg - I don't see any reason not to do this, but thought I'd check (this script was written to address the old reproject_events behavior where the TLMIN/MAX wouldn't be updated, but that's no-longer been true for a while now).

@kjg
Copy link

kjg commented Nov 5, 2020

I'm guessing this is intended for @kglotfelty?

@DougBurke
Copy link
Member Author

Oops - sorry @kjg

@kglotfelty
Copy link
Member

I think the clip part is kinda important. I get worried about the grids between events and exposure maps not matching up (since I think one starts at pixel center and the other at pixel edge -- going from memory on this). I just don't know the effect.

@DougBurke
Copy link
Member Author

For the fluximage like tasks I spent some time trying to make sure we always use "sensible" ranges for binning and image filtering (by extending to the outermost half-bin boundary on a scale that starts at 0.5 and uses the requested binning factor). So it should only be an issue for downstream tooling. That's not to say that's not important.

Because of this I hope the different tools are using the same scheme!

@kglotfelty
Copy link
Member

I was concerned about things like reproject_obs too , eg if you just dmcopy or even just load event file in ds9. But it looks like reproject_obs is also setting the range to nice half pixel boundaries.

@DougBurke
Copy link
Member Author

DougBurke commented Nov 30, 2020 via email

@kglotfelty
Copy link
Member

Yes, with this pr, but it’d be great to double check and make sure I didn’t foul it up.

@DougBurke
Copy link
Member Author

So, I think I'm probably not going to go with this at the moment. It does improve the code base, but only slightly, and I haven't done enough analysis to really know what the downstream consequences are.

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.

can we dump update_column_range
3 participants