finsberg / gotranx

Next generation ODE translator
https://finsberg.github.io/gotranx
MIT License
8 stars 1 forks source link

JOSS Review: Issues with tutorials #115

Closed ayush9pandey closed 2 months ago

ayush9pandey commented 2 months ago

When running through the Using the Python API and the Your first ODE file page, I am running into a few issues:

  1. I don't see a link to the ORdmm_Land.ode file. I was able to find it eventually under examples/split-ode/, so it might be better to mention the location on the documentation page.
  2. As a user not familiar with cardiac physiology models, it's difficult to understand the model filename or what it means. This tutorial is placed before the "Your first ODE file", so at this point it is difficult to understand .ode files for new users.
  3. Not clear what's printed when I save an ODE file (the number 294). image
  4. On similar lines as the points above, it might be helpful if there was a tutorial on integrating other ode solver schemes. Maybe this is already given somewhere..
finsberg commented 2 months ago

Thanks for your comments. I have now addressed all of these points here: https://github.com/finsberg/gotranx/pull/117 The only exception is point 3. 294 refers to the number of characters written to the file which is the return value from Path.write_text. I see two possible ways to fix this

  1. Assign the value value to a variable, i.e
    num_chars = Path("lorentz.ode").write_text(ode_str)

    which now introduces an unused variable. Alternatively, I could assign it to _, i.e

    _ = Path("lorentz.ode").write_text(ode_str)

    but I also think this looks a bit weired.

  2. I could write a sentence saying that 294 is just the number of characters. However, I think this also sounds a bit too much.

Do you have any opinions here?

ayush9pandey commented 2 months ago

I agree -- this is not a major issue either way. My approach in the past has been to create a package specific write_model function, which in your case could be write_ode(ode_string, filename). That way, you can wrap around the Path library's function and get the desired behavior.

finsberg commented 2 months ago

I agree -- this is not a major issue either way. My approach in the past has been to create a package specific write_model function, which in your case could be write_ode(ode_string, filename). That way, you can wrap around the Path library's function and get the desired behavior.

OK, I will just keep it as is. There is already a method for saving an ODE to a file (see https://finsberg.github.io/gotranx/docs/api.html#gotranx.ode.ODE.save). In this case I am simply just writing a string to a file and I see no good reason to why I should make a new function to do exactly the same thing (except not returning the number characters written).

ayush9pandey commented 2 months ago

OK. So, in principle, I could create an ODE object from the string and then use save to save it to a file instead of using the Path library directly?

finsberg commented 2 months ago

Yes, there is actually also a function for that, so I updated the example to use that in stead (see https://github.com/finsberg/gotranx/pull/119). Thanks