search

biomass-dev / biomass

A Python Framework for Modeling and Analysis of Signaling Systems
http://biomass-core.rtfd.io
Apache License 2.0
27 stars 13 forks source link

Allow different time points between conditions for experimental data #273

Closed k-arakane closed 4 months ago

k-arakane commented 4 months ago

It should be acceptable to have experimental data with different time points between conditions for the same observable (as in Raia et al., 2011), but currently it is only possible to vary the time points between the observables.

Here, I modified the get_timepoint() function to return a dictionary ({ "condition": list(tpoints) }) to offer more flexibility while trying not to complicate the code too much.

Due to this change, I needed to alter several parts of the code including get_timepoint() of the example model files. I also added a replication of Raia et al.'s JAK-STAT model to showcase an actual use case of this feature.

I would like to know what you think about changing the return type of get_timepoint(), because alternatively I think we can pass the condition name as one of the inputs to achieve the same effect.

k-arakane commented 4 months ago

Hello @himoto! Thanks for reviewing my code. My responses to your requests are as follows:

Regarding the second point, should I change the target branch to something other than master (like 0.14.0) so that all modifications can be checked beforehand? I'm not really familiar with these kind of procedures, so I would like to know. Thanks!

himoto commented 4 months ago

Hi @k-arakane, Thanks for your point-by-point answers!

updating the tutorial → I agree that the documentations need to be changed to match the updated code, and I would like to work on this

Many thanks, after you work on it, I will merge & close this PR.

bumping version to 0.14.0 → actually, I am planning to open a different PR that adds another relatively large feature (I will open a discussion about this later), so can we wait until that is ready for review as well?

No problem. When you are done adding all the new features, please bump the version.

Regarding the second point, should I change the target branch to something other than master (like 0.14.0) so that all modifications can be checked beforehand? I'm not really familiar with these kind of procedures, so I would like to know.

That's good idea. Can you please do the following?

  1. Create dev branch. This PR should also be merged there.
  2. After adding all features, create a PR, e.g., "Release v0.14.0" to the master branch.
k-arakane commented 4 months ago

Hi @himoto! I have another question regarding this change; the original get_timepoint function had a @staticmethod decorator, but I removed this in order to use the condition list within this function.

Was there a particular reason behind the original decision to define this function as a @staticmethod? I realized that removing this line wasn't strictly necessary, as we can hard code the condition names in this function just like how the observable names are currently are. I'd like to know your thoughts! Thank you in advance.

himoto commented 4 months ago

Hi @k-arakane, I can no longer clearly recall the specific reason why I used @staticmethod decorator there, but I am pretty sure it was not based on anything critical. If you think omitting it will allow us to be more flexible in specifying experimental timepoints, I'm more than happy to accept it!

k-arakane commented 4 months ago

Thanks for the reply, @himoto!

If that is the case, I guess we can remove the decorator so it is possible to access other information regarding the experimental data (like observable names) as well. This may especially come in handy when we try to implement the new feature discussed in #274.

k-arakane commented 4 months ago

Hey @himoto! I updated the documents to match the new code, and also address issue #275, could you have a look?

k-arakane commented 4 months ago

Great! Thanks again for taking the time to review my code.