search

lbl-anp / becquerel

Becquerel is a Python package for analyzing nuclear spectroscopic measurements.
Other
43 stars 16 forks source link

Added parser for IEC 1455 files #322

Closed werthm closed 2 years ago

werthm commented 2 years ago

Hi I'm happy to contribute to becquerel by adding a parser for IEC 1455 files. Data from all of the commercial acquisition software used at our institute can be exported into this format. Therefore a dedicated parser allows us to analyze data from different spectroscopy systems directly in becquerel. Hope you like it. Cheers, Dominik

jvavrek commented 2 years ago

Thanks for this @werthm! Do you mind fixing the flake8 errors in iec1455.py? And we can probably have pytest-black ignore the .iec files if I can remember how to modify the setup.cfg accordingly.

werthm commented 2 years ago

Hi, flake8 errors should be fixed now. Running all tests locally, I get errors (AttributeError: 'Spectrum' object has no attribute 'energies') in tests/parsers_test.py for all the test file extensions, not just .iec.

jvavrek commented 2 years ago

Hi, flake8 errors should be fixed now. Running all tests locally, I get errors (AttributeError: 'Spectrum' object has no attribute 'energies') in tests/parsers_test.py for all the test file extensions, not just .iec.

Interesting, your test environment must be ignoring the option to skip plot tests. But this shows that the plot tests, which are always (?) skipped can be quite out of date, so good catch. @markbandstra @jccurtis should we address this? IIRC the fix here is just spec.energies --> spec.bin_centers_kev

jvavrek commented 2 years ago

@werthm can you update the top of the top-level .pre-commit-config.yaml to the following:

# Exclude CNF, cnf, and iec files
exclude: '(?:\.cnf|\.CNF|\.iec)$'

and see if that ignores the .iec files correctly?

werthm commented 2 years ago

Done!

werthm commented 2 years ago

Any updates on merging this with the master?

markbandstra commented 2 years ago

Hi @werthm we're going to have a meeting tomorrow and will take a look. Thanks again for this contribution!

jvavrek commented 2 years ago

@werthm looks like you just need to re-run pre-commit (or black alone) and then we're good to go. Thanks for this contribution!

werthm commented 2 years ago

Ok guys, I do get that you want to have your source "nicely" formatted but allow me to say that contributing to this project has not been the nicest experience. I delivered valid code from the very beginning which for a non-developer user analyzing IEC files can be the key reason to choose becquerel over a different software. What about a maintainer fixing these trivial issues just before merging? I would like to continue contributing to your project rather than pushing to my own fork of becquerel. Sorry for letting off some steam...

markbandstra commented 2 years ago

Hi @werthm, apologies for your frustration with our process here, and thank you for your honest feedback. I consulted with @jccurtis and @jvavrek in my response here. We customarily require the author of a pull request to handle any requested changes, even if they seem trivial. Sorry for the mismatch in expectations here. We’ve tried to spell this out in the contribution guidelines and please let us know if they can be clearer.

Looking more closely into this PR, a lot of the specific test failures were around pre-commit formatting and excluding a new file extension (*.iec) from the pre-commit pipeline. We realized that it will save us all a lot of time to enable a CI hook on GitHub to automatically run pre-commit and push relevant fixes, which we tested here). This hook will only work for branches in the main repo so outside developer forks will need to make the fixes (run pre-commit run --all) themselves. Once a contribution is made we could add you to the dev team so you can push branches to the main repo.

The original intention behind this project was to pool our efforts so we each didn't have to reinvent the wheel when it comes to spectroscopic analysis, and we made it open source in case anyone else out there would find it useful too. We’re happy to see that you found it useful, and have contributed in return. This is as good as it gets in open source! On the other hand this is a side project for us and we don’t have much bandwidth, so we appreciate your patience with this process.

markbandstra commented 2 years ago

Merging despite test failures, which are unrelated. Those issues will be tracked by #332

werthm commented 2 years ago

Hi @markbandstra @jccurtis @jvavrek , thanks for your reply. I think the contribution guidelines could be a bit more detailed, especially for people unfamiliar with the various developing tools you're using. Please keep in mind that people might not be able to dedicate time to get into the various tools (for whatever reason). It would be a pity to loose new contributions just because of this.