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 coveralls to travis #290

Merged
merged 10 commits into from
Nov 25, 2020
Merged

Add coveralls to travis #290

merged 10 commits into from
Nov 25, 2020

Conversation

JoranAngevaare
Copy link
Contributor

What is the problem / what does the code in this PR do
Add https://docs.coveralls.io/ to travis testing.

Can you briefly describe how it works?
Just like in strax

My concern
I'm just copy pasting code here. I have added coveralls for straxen: https://coveralls.io/github/XENONnT/straxen, however I'm not sure if this is going to work the first time. If someone is willing to give me the green light I can just push to master.

I think this would be great to add because it shows how good/bad your PR is and increases the incentive to write tests.

@JoranAngevaare
Copy link
Contributor Author

JoranAngevaare commented Nov 23, 2020

@WenzDaniel , are we running into AxFoundation/strax#328 at line https://github.com/AxFoundation/strax/blob/master/strax/processing/pulse_processing.py#L116 at commit 77b2e18?

This just happens for numbaless coverage (newly added in this PR) so you probably have never seen it before.

afbeelding

@WenzDaniel
Copy link
Collaborator

WenzDaniel commented Nov 24, 2020

This is interesting, but no the Issue you are linking would be the opposite I think. Back then I had the issue that my assignment

data = thing['data'][thing['length']]

was a view onto thing['data']. Hence when I modified data, I indirectly modified thing['data'] which was not desired. Hence this read-only thing you are describing here would have saved me from this issue.
I know that one can make an array immutable via array.flag.writeable = False, but I do not think that we are explicitly doing this somewhere. Further, I checked the requirements since v0.12.0 and numpy and numba has not change since then. Have you updated your env recently?

@WenzDaniel
Copy link
Collaborator

WenzDaniel commented Nov 24, 2020

The thing I do not understand though, why is it just popping up for the nveto_recorder?

Okay, I understand why, funny. Here is what happened and now happens:

The arrays we are returning in the compute method of our plugins are read-only which makes sense. In the past there was a bug in numba that a njited function which work on a immutable structure array would just do nothing (see numba/numba#5950). However, it looks like that they have changed this recently and now numba throws an error.

Since the function was not doing anything before, and I do not see that it would make any difference, we can just remove it.

@JoranAngevaare
Copy link
Contributor Author

Thanks @WenzDaniel , trying to work from here now

@JoranAngevaare
Copy link
Contributor Author

JoranAngevaare commented Nov 24, 2020

Ok, seems to work :) @ershockley or @WenzDaniel willing to review (is just technical so might do without). Maybe to save some time on travis tests, I will disable py3.7 testing and just to numbaless py3.7. Any opinions?

@daniel, 29% oopf, we have work to do..

@JoranAngevaare
Copy link
Contributor Author

Ok, changed the way it looks at the package, it's now at 61 percent (apparently this test_plugins is very effective). I will continue and merge.

@JoranAngevaare JoranAngevaare merged commit 0c0e6b2 into master Nov 25, 2020
@JoranAngevaare JoranAngevaare deleted the add_coveralls branch November 25, 2020 11:52
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