8000 Remove em-specific code that is becoming replaced by pynxtools-em by mkuehbach · Pull Request #289 · FAIRmat-NFDI/pynxtools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove em-specific code that is becoming replaced by pynxtools-em #289

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

Merged
merged 5 commits into from
Mar 19, 2024

Conversation

mkuehbach
Copy link
Collaborator

No description provided.

atomprobe-tc and others added 3 commits March 18, 2024 19:03
…ed by the pynxtools-em plugin, implementing a contribution for #283
Remove em-specific dataconverters as their functionality is superseed…
@mkuehbach mkuehbach requested review from domna and sanbrock March 19, 2024 08:31
@coveralls
Copy link
coveralls commented Mar 19, 2024

Pull Request Test Coverage Report for Build 8340532143

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 8 unchanged lines in 5 files lost coverage.
  • Overall coverage increased (+26.8%) to 76.558%

Files with Coverage Reduction New Missed Lines %
pynxtools/nexus/nxdl_utils.py 1 73.49%
tests/dataconverter/test_readers.py 1 92.59%
pynxtools/dataconverter/template.py 2 83.74%
pynxtools/dataconverter/convert.py 2 74.86%
pynxtools/dataconverter/helpers.py 2 91.14%
Totals Coverage Status
Change from base Build 8294211401: 26.8%
Covered Lines: 2789
Relevant Lines: 3643

💛 - Coveralls

Copy link
Collaborator
@domna domna left a comment

Choose a reason for hiding this comment

8000

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

The removal of files look all good to me.
I went through the file and checked which imports we actually use and I found that we don't use these packages anymore:

  • flatdict
  • gitpython
  • zipfile37
  • tzlocal
  • scipy

I don't know whether these packages were used by you or if they were removed previously. However, I think we can savely remove them from the dependencies now.

There is also a shared folder in dataconverter/readers which is not used anymore. Shall we remove this as well. Otherwise I would suggest we move the functions in dataconverter/utils.py. If this will be removed we can also remove the pytz dependency in #285

@mkuehbach
Copy link
Collaborator Author

Getting consistent code for managing time stamps and zone rigorously may demand using the pytz library, in one case of em it even required tzlocal but we should give it a test, so please remove pytz via #285, right the shared directory I missed I will also remove it now

Copy link
Collaborator
@domna domna left a comment

Choose a reason for hiding this comment

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

Thanks LGTM now

@mkuehbach mkuehbach merged commit e214bfd into master Mar 19, 2024
@mkuehbach mkuehbach deleted the mmov_em_apm_announcement branch April 16, 2024 20:04
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.

4 participants
0