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

Retrieve matrix #28

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

JohannesBuchner
Copy link

Hi @dhuppenkothen,

thanks for this cool project!
I wanted to just get the RMF matrix values out.

In one commit I also played with using scipy.sparse to create sparse matrices, but I found that their .dot is not faster than just using the full matrix. So I removed the code again (in the second commit).

Cheers,
Johannes

According to my testing, it is not faster than just using dense
matrices. That might be because our arrays are not huge O(5000x5000).

In [22]: %timeit numpy.dot(spec * aarf.specresp, m)
100 loops, best of 3: 2.9 ms per loop

In [21]: %timeit ms.dot(spec * aarf.specresp)
The slowest run took 8.49 times longer than the fastest. This could mean that an intermediate result is being cached.
100 loops, best of 3: 3.12 ms per loop
@dhuppenkothen
Copy link
Owner

Ooh, thanks for this! This is really helpful. Travis is currently failing to reasons not related to this PR, so I'll have to investigate what's going on there.

Relatedly, do you think you might have a spare few minutes to come up with a couple of test cases to make sure this executes correctly? That would be awesome!

@JohannesBuchner
Copy link
Author

Heh, I was afraid you would ask for that. How should I test the correctness? Would it be OK to compare apply_rmf against the dot product with the dense matrix, or is that too high-level?

I saw in another PR that apply_rmf (times arf) might not correspond to proper fluxes at the moment, because some keywords (exposure time, scalings?) may be needed. Is that still true?

@JohannesBuchner
Copy link
Author

Added a test case. I did not test the test yet, haven't downloaded git-lfs.

@dhuppenkothen
Copy link
Owner

Yeah, I think comparing to the apply_rmf sounds fine. I'm trying not to over-engineer this package. :)

I haven't looked at this package in quite a while, to be honest so the response to your second question is ... probably? I'll check either today or tomorrow and get back to you.

@dhuppenkothen
Copy link
Owner

git-lfs has been producing lots of hiccups recently; if it doesn't work, let me know and I might have to put the data elsewhere for easier download.

@JohannesBuchner
Copy link
Author

Hi @dhuppenkothen, are you still considering merging this?

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.

2 participants