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

Refactor #248

Merged
merged 7 commits into from
Apr 21, 2022
Merged

Refactor #248

merged 7 commits into from
Apr 21, 2022

Conversation

iguinn
Copy link
Collaborator

@iguinn iguinn commented Apr 1, 2022

  • Resolved merge conflicts. build_dsp works
  • Added some missing init files
  • Moved metadata directory of json files from tutorials to the project tree. These are now installed and can be accessed using with importlibs.resources.path("pygama.metadata", "file.json") as p: ...
    • Tutorials need to be updated still
  • Added keyword arguments for "args" lists. Also made some fixes so that None and string values can be passed
  • Some processors have time values passed as floats that are then checked for integer values. This causes problems for the refactor branch, since unit conversions often introduce roundoff errors, and unitted numbers cannot be rounded easily. I will create an issue for this.

@gipert
Copy link
Member

gipert commented Apr 1, 2022

We should merge this after #241, there are some conflicts between the two. Just couple of comments from my side concerning packaging:

I would strongly recommend to keep user code separate from the pygama codebase, so I would not move those JSON files from tutorials to pygama.metadata. I think we should retain only code that is needed for pygama to work and keep apps/examples at least in a separate directory outside the package tree – but I would actually be much happier with a separate Git repository, something like pygama-contrib. We can add this as a discussion item at the next pygama call, I wanted to bring this issue up anyways.

About third-party, non-python code like Eigen: I think this should also be placed elsewhere, maybe in a folder at the repo root dedicated to third party projects. We should think about it also install-wise. I'm also usually more in favor of directly mirroring the code in our repository, instead of using Git submodules, which are sometimes painful to handle. Let's discuss this too next time we meet!

@gipert gipert added the dsp Digital Signal Processing label Apr 6, 2022
@iguinn
Copy link
Collaborator Author

iguinn commented Apr 6, 2022

Thanks for the comments, Luigi. I've:

  • Removed eigama; that was a mistaken inclusion, and it remains on a separate branch (as you say, there are a lot of open questions about how to include it)
  • Reverted my move of metadata into the main package
  • Removed the dsp README file, and expanded the init documentation a little bit

As for the remaining merge conflicts, once we get #241 in, I can revisit the merge conflicts.

pygama/dsp/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Luigi Pertoldi <gipert@pm.me>
@jasondet
Copy link
Collaborator

@iguinn we are ready for you to revisit this

@jasondet jasondet merged commit edac6c6 into legend-exp:refactor Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dsp Digital Signal Processing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants