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

Docs housekeeping #715

Merged
merged 28 commits into from
Dec 3, 2023
Merged

Docs housekeeping #715

merged 28 commits into from
Dec 3, 2023

Conversation

TorkelE
Copy link
Member

@TorkelE TorkelE commented Nov 8, 2023

  • Fixes a spelling mistake.
  • Changes usage of to -->.
  • Adds in figure files that somehow was missing from the PEtab doc, sorry about that.

@isaacsas
Copy link
Member

isaacsas commented Nov 8, 2023

Why aren't the figures being generated dynamically as an example block?

@TorkelE
Copy link
Member Author

TorkelE commented Nov 8, 2023

I have an example block before. However, given that it is re-run every time the docs happen, and that there is a potential chance that the parameter fitting fails (possible especially when we make the initial example using a single run), making the doc look funny, I simply pasted in output figures from a run I made. That way there is no risk they change every time.

@isaacsas
Copy link
Member

isaacsas commented Nov 8, 2023

But if that is the case isn't this a bad example to ask a new user to try running themselves? If they run it and it fails they will likely be quite confused and assume they've made a mistake.

@TorkelE
Copy link
Member Author

TorkelE commented Nov 9, 2023

If it was an example where that happened frequently, I would agree. I just did 25 runs of the fitting and they all turned out fine, this is mostly me being paranoid. Ideally (and especially for difficult problems) one should use multistart-optimization, however, I wanted to build things up from the beginning with the simplest case, and not overload the user. If you want I could add a note of this.

@TorkelE
Copy link
Member Author

TorkelE commented Nov 9, 2023

I do not understand this issue on the 1.6 CI that has started to pop-up:
image

Otherwise, this is all good to go now.

@TorkelE TorkelE closed this Nov 11, 2023
@TorkelE TorkelE reopened this Nov 11, 2023
@isaacsas
Copy link
Member

Looks like CI is passing now, but I would still strongly prefer we keep example code as @examples as done in most of the rest of the docs. If you need a deterministic answer can you not set the random number seed and use StableRNGs? You could mention this choice is just to ensure the simulation gives the same result in the docs as the fitting involves randomness as setup, and users should just skip it (or hide that code block from users).

I just think we get a lot of value from being able to see that a doc is suddenly not working, which we lose when things aren’t @examples blocks.

@TorkelE
Copy link
Member Author

TorkelE commented Nov 12, 2023

So, I definitely agree that the automatic testing that @example provides is invaluable, and we were burned before that system was put in place. I will think if I can make it possible for the first case. I tried to maintain @example where possible (even if I set the display myself) to catch errors at least. There are however some cases like:

using Optim
using QuasiMonteCarlo
res_ms = calibrate_model_multistart(petab_problem, IPNewton(), 10, "OptimizationRuns"; sampling_method=QuasiMonteCarlo.SobolSample())

where the code actually saves the output to a file. I did not want our doc code to start saving stuff to the documentation, hence this was disabled.
(later on I also load stuff from this "file", which for the same reason I disabled)

@isaacsas
Copy link
Member

If the files are small we could have them saved into the assets folder dynamically. But I’m ok with files that are saved or loaded being premade. I’d prefer figures are dynamic though since they are easy to scan to see doc problems.

@isaacsas
Copy link
Member

Typically I find problems by noticing the absence of figures…

@TorkelE
Copy link
Member Author

TorkelE commented Nov 12, 2023

yeah, this way it is only really showing up in the doc logs when you happen to have a look at it, so def not perfect (and there might also be problems that do not involve throwing an error as well)

@TorkelE
Copy link
Member Author

TorkelE commented Nov 12, 2023

If the files are small we could have them saved into the assets folder dynamically. But I’m ok with files that are saved or loaded being premade. I’d prefer figures are dynamic though since they are easy to scan to see doc problems.

I could add a hidden line that directly deletes the generated folder
(PEtab doesn't overwrite, so will keep adding sub files to the target folder each time it is run)

@isaacsas
Copy link
Member

Yes, we don't want to keep adding new folders everytime this runs within the doc hierarchy. If you can tell PETab where to create it though you can just use tempname to have Julia give you a temporary name that should get cleaned up (but it shouldn't get saved on the Github website branch).

@isaacsas
Copy link
Member

Or make sure to delete and overwrite the old sub files/folders.

@TorkelE
Copy link
Member Author

TorkelE commented Nov 16, 2023

I could add a hidden line (before the line which saves the data to the folder), which create the folder (and then delete the folder later in the routine.

@isaacsas
Copy link
Member

Sure

@TorkelE
Copy link
Member Author

TorkelE commented Nov 16, 2023

Updated (but just want to check that it all builds nicely before merging).

TorkelE and others added 3 commits December 3, 2023 17:22
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@TorkelE TorkelE merged commit 38ea201 into master Dec 3, 2023
2 of 5 checks passed
@TorkelE TorkelE deleted the docs_housekeeping branch June 8, 2024 18:30
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.

None yet

2 participants