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

Fix visualization w/o condition names #142

Closed
MerktSimon opened this issue Sep 30, 2019 · 7 comments
Closed

Fix visualization w/o condition names #142

MerktSimon opened this issue Sep 30, 2019 · 7 comments
Labels
bug Something isn't working good first issue Good for newcomers visualization

Comments

@MerktSimon
Copy link
Member

https://github.com/ICB-DCM/PEtab/blob/367a0d1d893b30ec6d16d91122a148c96034d782/petab/visualize/helper_functions.py#L168

In the PEtab format the column "conditionName" is optional. If one chooses to leave this (empty) and does not us a visualization specification file, an "AttributeError: 'DataFrame' object has no attribute 'conditionName'" rises.

@MerktSimon MerktSimon added bug Something isn't working good first issue Good for newcomers labels Sep 30, 2019
@yannikschaelte
Copy link
Member

@paulstapor I think if the condition name does not exist, we should use a default "Condition n", ok? @MerktSimon to implement this, one could either define a function which "normalizes" the input in the first function called by the user, i.e. adds default values/columns for missing values. Or, whenever needed, you check whether the non-mandatory column exists before using it.

@JanHasenauer
Copy link
Contributor

What about using the conditionID? This would provide a unique identifier.

@yannikschaelte
Copy link
Member

Right. Also, a good thing in general would be to, if a name is needed but none exists, then take the id instead (that has to exist always? if not, and it does not exist, then resort to some generic "Condition n").

@JanHasenauer
Copy link
Contributor

The ID has to exist for the mapping. We should enforce this and return an error message otherwise.

@MerktSimon
Copy link
Member Author

It already does return a KeyError: 'Condition table missing mandatory field 'conditionId' if a conditionId does not exist.
The part of code mentioned above is intended to # create nicer legend entries from condition names instead of IDs
So I guess the question is if "Condition n" is in general that much easier on the eye than the conditionID. (Or a way to prevent legends blowing up due to lengthy Ids?)

@yannikschaelte
Copy link
Member

No, I would not prevent anything from popping up, and just use the id. If the user is not happy, they will have to provide names, imho.

@paulstapor
Copy link
Contributor

No, I would not prevent anything from popping up, and just use the id. If the user is not happy, they will have to provide names, imho.

Agree with it.
ConditionIDs must exist. If names are not present, IDs are the most reasonable default here. I would rather not try to make up something else...

MerktSimon added a commit that referenced this issue Dec 9, 2019
dweindl added a commit that referenced this issue Dec 17, 2019
Release 0.0.1

Data format:
* Update format and documentation with respect to data and parameter scales
  (#169)
* Define YAML schema for grouping PEtab files, also allowing for more complex
  combinations of files (#183)

Library:
* Refactor library. Reorganize `petab.core` functions.
* Fix visualization w/o condition names #142
* Extend validator
* Removed deprecated functions petab.Problem.get_constant_parameters
  and petab.sbml.constant_species_to_parameters
* Minor fixes and extensions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers visualization
Projects
None yet
Development

No branches or pull requests

5 participants