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

BUG: use np instead of pd in SpectraCompiler. #60

Merged
merged 5 commits into from
Aug 28, 2020

Conversation

findesgh
Copy link
Contributor

This is an alternative to #59, either fixes not being able to load for me. This one uses numpy exclusively pandas.

@maxbriel
Copy link
Collaborator

Hi findesgh,

Thank you for the different implementation! The SpectraCompiler issue is solved using the pandas method at the moment. Did you already have a look at the performance this method vs the pandas one?

Since the tests for the SpectraCompiler are a little tricky, I will have a look at them to make them work for this method.

@maxbriel maxbriel self-requested a review August 28, 2020 21:13
@findesgh
Copy link
Contributor Author

Yes, I noticed that a few other tests are failing as well. Now I hope that they should pass.

I haven't compared the performance, no. I just think using numpy is a bit easier in this case. Just a suggestion, though.

@maxbriel maxbriel merged commit 30485f5 into HeloiseS:csp Aug 28, 2020
Copy link
Collaborator

@maxbriel maxbriel left a comment

Choose a reason for hiding this comment

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

The numpy method looks cleaner and less bloated, so I agree! With the updates test, I have merged it with the csp branch.
Thank you for the contribution!

@findesgh findesgh deleted the csp_fix2 branch October 2, 2020 16:25
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.

None yet

2 participants