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

Add calib_astrometryUsed flag to AstrometryTask #71

Merged
merged 1 commit into from May 10, 2017
Merged

Conversation

pgee2000
Copy link
Contributor

@pgee2000 pgee2000 commented May 8, 2017

Previous merge was accidental. The actual changes are here.
Includes a quick test which only shows that the task is able
to write to the schema and sources table.

@laurenam
Copy link
Contributor

laurenam commented May 8, 2017

I still don't see where the import random in astrometry.py is being used.

Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

Just a couple of comments this time.

config = AstrometryTask.ConfigClass()
config.wcsFitter.order = 2
config.wcsFitter.numRejIter = 0
# schema must be passed to the solver task constructor
Copy link
Contributor

Choose a reason for hiding this comment

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

only one space after hash.

sourceCat=sourceCat,
exposure=self.exposure,
)
# check that the used flag is set the right number of times
Copy link
Contributor

Choose a reason for hiding this comment

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

only one space after has.

# check that the used flag is set the right number of times
count = 0
flagKey = sourceCat.getSchema().find("calib_astrometryUsed").key
for source in sourceCat:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this for loop can be just count = (sourceCat['calib_astrometryUsed']==True).sum()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that I originally I tried this, or a slight variant, as I did in the photCal test. But I got 'Record data is not contiguous in memory. ' error. I did not stop to explore it, but just changed it to a loop.
Let me know if you think it matters. Since I built this out of a test which already existed, I didn't particularly want to mess around with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised that it won't work in this particular context, but I haven't tried. Since it's in the test code, I guess it's fine.

@pgee2000
Copy link
Contributor Author

pgee2000 commented May 8, 2017

I also removed the import random as per Lauren comment. It was used by the reserve candidates code, which was later removed.

@pgee2000 pgee2000 force-pushed the tickets/DM-9050 branch 2 times, most recently from c92b862 to ee69e0b Compare May 10, 2017 01:21
Previous merge was accidental.  The actual changes are here.
Includes a quick test which only shows that the task is able
to write to the schema and sources table.
@pgee2000 pgee2000 merged commit ee69e0b into master May 10, 2017
@ktlim ktlim deleted the tickets/DM-9050 branch August 25, 2018 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants