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

Improve image pipe class #524

Merged
merged 20 commits into from May 3, 2016
Merged

Conversation

@JouvinLea
Copy link
Contributor

@JouvinLea JouvinLea commented May 2, 2016

@cdeil @adonath
Hi,
I made some changes. We have now two classes: one for one observation ObsImage and one for a set of Observation MosaicImage.

We discussed with regis and this is less time computing... It is also much more clear to have these two separate classes!

@JouvinLea
Copy link
Contributor Author

@JouvinLea JouvinLea commented May 3, 2016

@adonath All the test passed so if you agree with what I changed we can merge!!
It's really the same ideas and algorithm than the image analysis class but just separated in two classes!

@adonath adonath self-assigned this May 3, 2016
self.data_store = data_store

self.obs_table = obs_table
def __init__(self, events, data_store, empty_image,

This comment has been minimized.

@adonath

adonath May 3, 2016
Member

Minor suggestion: can you slightly change the class, so that it takes a DataStoreObservation object instead of the event list and the data store? I think this makes in the end a clearer API:

obs = data_store.obs(obs_id)
image_analyis = ObsImage(obs, ...)

self.maps["total_counts"] = total_counts
return total_counts
print("The total number of events {} is inferior to the requested minimal counts number".

This comment has been minimized.

@adonath

adonath May 3, 2016
Member

Can you change this to either use log.warn('Too few counts...'), when you want the program to continue or raise an error using raise ValueError('Too few counts...'), when it should stop.

The logging module can be used, when you put the following at the top of the file:

import logging
log = logging.getLogger(__name__)
@adonath
Copy link
Member

@adonath adonath commented May 3, 2016

@JouvinLea It's definitely an improvement to re-factor the code into two classes. I've left two minor comments for this PR. Let me know if you still can include these and than we can merge later today...

data_store.hdu_table.remove_row(14)
mosaic_images = MosaicImage(image, energy_band=energy_band, offset_band=offset_band, data_store=data_store,
obs_table=data_store.obs_table, exclusion_mask=exclusion_mask)
mosaic_images.make_images(make_background_image=True, for_integral_flux=True, radius=10.)

This comment has been minimized.

@adonath

adonath May 3, 2016
Member

Can you add at least some assert statements here, testing on the sum of the data? E.g.:

from numpy.testing.utils import assert_equal
assert_equal(mosaic.maps['counts'].data.sum(), 42)
assert_equal(mosaic.maps['background'].data.sum(), 84.123456)
assert_equal(mosaic.maps['significance'].lookup(crab_position), 50.3455)
...

This will pin the status of the code to the version you are using right now and make your (science) results reproducible.

@JouvinLea
Copy link
Contributor Author

@JouvinLea JouvinLea commented May 3, 2016

@adonath
Thanks for your suggestions!! I tool all of them into account !!

@adonath
Copy link
Member

@adonath adonath commented May 3, 2016

Some of the tests still fail on Travis-CI:

E       AssertionError: 
E       Items are not equal:
E        ACTUAL: 1987.2088283447931
E        DESIRED: 1987.2088283446637

But these are only numerical differences, changing to assert_almost_equal() should fix this. If necessary you can adjust the precision of the test with the argument decimal=... (see here).

EDIT: Or maybe change to assert_allclose() see here, which is recommended by numpy.

@JouvinLea
Copy link
Contributor Author

@JouvinLea JouvinLea commented May 3, 2016

ok this is weird because it passes for me locally
Le 3 mai 2016 à 12:05, adonath notifications@github.com a écrit :

Some of the tests still fail on Travis-CI:

E AssertionError:
E Items are not equal:
E ACTUAL: 1987.2088283447931
E DESIRED: 1987.2088283446637
But these are only numerical differences, changing to assert_almost_equal() should fix this. If necessary you can adjust the precision of the test with the argument decimal=... (see here).


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

Lea Jouvin
@adonath
Copy link
Member

@adonath adonath commented May 3, 2016

I think these are just numerical differences, that can occur on different machines. So changing to assert_allclose() or assert_almost_equal() is perfectly fine.

@JouvinLea
Copy link
Contributor Author

@JouvinLea JouvinLea commented May 3, 2016

@adonath
This is still failing on travis for the counts now bu me I have 2334 counts locally so I cant' change this test... I don't understand why this is 2333 on travis...

@adonath
Copy link
Member

@adonath adonath commented May 3, 2016

@JouvinLea I just checked out your branch and the test fails on my machine locally as well (yielding 2333 instead of 2334). All the other asserts fail as well. Could you check the numbers you computed and used for the test again? Maybe you have any local un-commited changes...

Otherwise I'd be fine with commenting the asserts out for now and add a comment, that this fails on some machines, for unknown reasons....

@JouvinLea
Copy link
Contributor Author

@JouvinLea JouvinLea commented May 3, 2016

@adonath
I tested again, on my machine it failes if I put 2333... This is really weird now? Maybe it comes from the GAMPPY_EXTRA variable? I fetched the latest version of gammapy-extra on my machine...
So we comment the lines for now with a TODO?

@adonath
Copy link
Member

@adonath adonath commented May 3, 2016

@JouvinLea That's indeed weird. So just comment it out and leave a TODO.

Lea Jouvin
@JouvinLea
Copy link
Contributor Author

@JouvinLea JouvinLea commented May 3, 2016

@adonath all the test passed, we can merge?

@adonath adonath merged commit 2ee2447 into gammapy:master May 3, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adonath
Copy link
Member

@adonath adonath commented May 3, 2016

Merged, thanks!

@cdeil cdeil added this to the 0.5 milestone May 3, 2016
@cdeil cdeil changed the title Image pipe: Reorganisation of the class Improve image pipe class May 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.