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

Notebook cleanup #44

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Notebook cleanup #44

wants to merge 25 commits into from

Conversation

EzryStIago
Copy link
Collaborator

To address issue #43

Much of the notebook's code has been removed to a helper module. Data are now encapsulated in dataclasses making the user interface much simpler.

@EzryStIago EzryStIago requested review from jhenin and gbrannigan April 15, 2024 02:10
@gbrannigan gbrannigan marked this pull request as draft April 15, 2024 12:03
Copy link
Owner

@jhenin jhenin left a comment

Choose a reason for hiding this comment

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

I've fixed the parameter order when creating the FEPData objects, swapping detectEQ and the name.

Then I get this error:
TypeError: plot_general() got an unexpected keyword argument 'pdf_type' because this relies on branch add_replicas of the SAFEP repo (possibly an updated version that is not on the public repo, as the name of that option is PDFtype in the version I have).

We need to merge that branch before we merge this.

@EzryStIago
Copy link
Collaborator Author

@jhenin I have opened a PR with those changes over in the safep repo: BranniganLab/safep#41

@jhenin jhenin self-requested a review May 7, 2024 16:02
Copy link
Owner

@jhenin jhenin left a comment

Choose a reason for hiding this comment

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

Good for me, pending the merge of BranniganLab/safep#41

@EzryStIago EzryStIago marked this pull request as ready for review May 9, 2024 15:13
@jhenin
Copy link
Owner

jhenin commented Dec 3, 2024

With current SAFEP module, I get:

TypeError: plot_general() got an unexpected keyword argument 'PDFtype'

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.

2 participants