-
Notifications
You must be signed in to change notification settings - Fork 5
Enhancement: Integrate settings upload in Data tab #860
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
base: main
Are you sure you want to change the base?
Enhancement: Integrate settings upload in Data tab #860
Conversation
…9-enhancement-integrate-settings-upload-button-with-data-upload
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @js3110
Thanks for the implementation, it looks good, just a few things to fix:
- Duplicate Rows Detected, this has to be done again, it's not kept in the settings
- buttons (in the exploration plots) and buttons (in the settings: choose NCA profile, etc.) are not kept when the settings are downloaded.
- The slope selector point exclusion is not kept
The pk parameters are working. (except when the NCA is not run)
I think we miss a general update of all the buttons and the parameters from the settings file, because when I open the settings file, I can see that I have all I changed inside.
Hi @Shaakon35 , the duplicate rows and exploration plot buttons are currently not in the settings, so these are not part of this PR. We can open separate issues to add these to settings (similar to #721 ) I will look into slope selection as this should be included. What do you mean the PK parameters are working except when the NCA is not run? |
|
@js3110 The pk parameter selection works, sorry about that |
|
@Shaakon35 I updated it, should be working with all the NCA settings now (setup, manual slopes, parameters). PTAL! |
|
@js3110
|
|
merged to include exclusions, but not yet in settings upload processing. Opened new issue #898 |
|
thanks @m-kolomanski , I updated based on your comments. Although some become redundant again when we switch to yaml (#901 ) |
Gero1999
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue
Closes #719
Description
Updated data tab upload so that settings files can also be uploaded and read correctly, and then carried into the NCA tab.
Definition of Done
How to test
Try uploading variations of datasets / settings etc to see what works and what doesn't work. If something doesn't work, it should give a clear error message to the user.
Contributor checklist
Notes to reviewer
Anything that the reviewer should know before tacking the pull request?