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

Updated signal subclassing and metadata of .sur files #98

Merged
merged 7 commits into from
Apr 26, 2023

Conversation

Attolight-NTappy
Copy link
Contributor

@Attolight-NTappy Attolight-NTappy commented Mar 30, 2023

Description of the change

Same as hyperspy/hyperspy#3120

Progress of the PR

  • Change implemented (can be split into several points),
  • add an changelog entry in the upcoming_changes folder (see upcoming_changes/README.rst),
  • Check formatting changelog entry in the readthedocs doc build of this PR (link in github checks)
  • add tests,
  • ready for review.

@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Patch coverage: 97.60% and project coverage change: +0.24 🎉

Comparison is base (854334d) 84.87% compared to head (083b3cd) 85.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
+ Coverage   84.87%   85.12%   +0.24%     
==========================================
  Files          73       73              
  Lines        8928     8988      +60     
  Branches     1962     1967       +5     
==========================================
+ Hits         7578     7651      +73     
+ Misses        884      875       -9     
+ Partials      466      462       -4     
Impacted Files Coverage Δ
rsciio/digitalsurf/_api.py 78.66% <97.60%> (+7.23%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rsciio/digitalsurf/_api.py Fixed Show resolved Hide resolved
rsciio/digitalsurf/_api.py Fixed Show resolved Hide resolved
Copy link
Contributor

@jlaehne jlaehne left a comment

Choose a reason for hiding this comment

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

Thanks @Attolight-NTappy - the rsciio implementation is currently failing. Might just be the docstring defaults we added that got lost in this PR.

rsciio/digitalsurf/_api.py Outdated Show resolved Hide resolved
rsciio/digitalsurf/_api.py Show resolved Hide resolved
@jlaehne
Copy link
Contributor

jlaehne commented Apr 17, 2023

There are still three failures in the tests.

@Attolight-NTappy
Copy link
Contributor Author

Should be solved now

@jlaehne
Copy link
Contributor

jlaehne commented Apr 19, 2023

Looks good, just lint is unhappy, while all the tests run through.

In principle, the reader would benefit from a higher coverage (with <80% it is below average for RosettaSciIO: #60), but that could be left for a separate PR.

@Attolight-NTappy
Copy link
Contributor Author

In principle, the reader would benefit from a higher coverage (with <80% it is below average for RosettaSciIO: #60), but that could be left for a separate PR.

Let's see how this goes now I have added a few checks (not much though)

@jlaehne
Copy link
Contributor

jlaehne commented Apr 25, 2023

Could you merge in main @Attolight-NTappy to see whether #106 fixed the currently failing tests?

@Attolight-NTappy
Copy link
Contributor Author

I did the rebase, let's see

@Attolight-NTappy Attolight-NTappy mentioned this pull request Apr 25, 2023
4 tasks
@jlaehne
Copy link
Contributor

jlaehne commented Apr 25, 2023

Let's see how this goes now I have added a few checks (not much though)

Well, already brings coverage from 78% to 85%:
https://app.codecov.io/gh/hyperspy/rosettasciio/commit/b068822a740d3e60fd4e0d6e0410d4370a2e4787

@ericpre
Copy link
Member

ericpre commented Apr 25, 2023

@jlaehne, are you happy with merging this PR?

@jlaehne jlaehne merged commit 2ab7611 into hyperspy:main Apr 26, 2023
@jlaehne jlaehne added this to the v0.1.0 initial release milestone Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants