Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
aa1cbcb to
e963d6b
Compare
9efc271 to
297aedc
Compare
|
@Scienfitz done, built docs here: https://hrovatin.github.io/baybe/latest/ |
AVHopp
left a comment
There was a problem hiding this comment.
Only one very minor thing, but other than that I'd say merge 🚀
There was a problem hiding this comment.
why has this figure been touched? optimize or something?
(we'll have to redo all figures once we release new priors in the next release anyway)
There was a problem hiding this comment.
It seems it was missed from the miniPR https://github.com/emdgroup/baybe/pull/726/changes
I can make one more miniPR with that image specifically @Scienfitz
What I changed is that it switches automatically based on browser defaults - before it was anyway static lightmode, so even if this does not work with website toggle, it is still better than before.
Ideal in future you could make it colored so that it works on light/dark without switch
There was a problem hiding this comment.
wait this a figure coming from the full lookupe xamnple, it is automatically created when running that example in fulltest mode
have yu made any cahgens to that? did you manually add or modify it somehow? I dont think the procedure how pictures from examples are generated should be changed by this PR, especially if its not automated
There was a problem hiding this comment.
yes, I made manual changes by merging the two light and dark mode versions into single file.
I reverted the change (removed the new plot) for now, using only light version, and we can later decide if we want to automate it.
There was a problem hiding this comment.
i dont really support this manual change because
- there is other doc figures for which you've not done it (outside of readme and this picture also originates from outside thee readme)
- your changes will be swiftly overwritten in case someone else redoes the figures, eg when we release the new version because this picture is the result of an example
So I see two ways forward
- do not do such a manual change on figures that stem from outside the readme
- put the logic you used to modify this picture in the relevant place so it is automatically repeated and not forgotten, I am not 100% sure but maybe the helper function
create_example_plotsfromexamples/utils.pywhich is used in the examples can be modified?
There was a problem hiding this comment.
@Scienfitz I would suggest that we move discussion on automating light+dark merging to a different issue, if required (since I removed it now). And for now (this PR) do one of the following:
- a.) Use the light only figure (current PR version).
- b.) I change the script that generates the plot to generate a gray version (besides light and dark) that we use in readme. I think this may be even better than merging as it is most robust (merging does not work with the light/dark toggle on the website).
This PR tackles some points from #598, focused on #598 (comment) as agreed on the team meeting
Link to the built documentation
I still have issues with displaying images, will investigate this.