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

fix raw option in morph_stat #964

Merged
merged 9 commits into from Sep 14, 2021
Merged

fix raw option in morph_stat #964

merged 9 commits into from Sep 14, 2021

Conversation

arnaudon
Copy link
Contributor

@arnaudon arnaudon commented Sep 14, 2021

When raw option is used, it crashed because np.raw does not exist.

@arnaudon
Copy link
Contributor Author

I'm adding the raw key in test for coverage

@arnaudon
Copy link
Contributor Author

@asanin-epfl , why not .tolist?

@asanin-epfl
Copy link
Contributor

@arnaudon and why .tolist ?

@asanin-epfl
Copy link
Contributor

We are waiting for @arnaudon coverage tests on 'raw' mode.

@arnaudon
Copy link
Contributor Author

here you are

}
}

REF_OUT = {
'morphology': {
'mean_soma_radius': 0.13065629648763766,
'mean_max_radial_distance': 99.5894610648815,
'raw_max_radial_distance': 99.5894610648815,
Copy link
Contributor

Choose a reason for hiding this comment

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

so, raw_max_radial_distance and mean_max_radial_distance are the same? is it ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is just one number here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can put raw in another entry if we want, but it may yield many numbers

@arnaudon
Copy link
Contributor Author

I don't know how to fix coverage...

@asanin-epfl
Copy link
Contributor

ok, I will fix the coverage

@codecov-commenter
Copy link

Codecov Report

Merging #964 (1cd1f17) into master (f699975) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #964   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           36        36           
  Lines         2136      2138    +2     
=========================================
+ Hits          2136      2138    +2     

@arnaudon
Copy link
Contributor Author

Thank you!

@@ -233,12 +241,16 @@ def test_extract_dataframe_multiproc():
for name in ['Neuron.swc', 'simple.swc']]
with warnings.catch_warnings(record=True) as w:
actual = ms.extract_dataframe(morphs, REF_CONFIG, n_workers=2)
# drop raw features as they require too much test data to mock
Copy link
Contributor

Choose a reason for hiding this comment

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

@adrien-berchet please confirm whether you are ok with such approach. I use in in 4 places here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it's fine, we don't care about the numbers, this is tested elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

It's ok for me too

@asanin-epfl asanin-epfl merged commit 6f4c23a into master Sep 14, 2021
@asanin-epfl
Copy link
Contributor

asanin-epfl commented Sep 14, 2021

do you need a release?

@arnaudon
Copy link
Contributor Author

no, it's fine! I just noticed when I was trying something locally, don't bother!

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

4 participants