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 visual issue in documentation by reducing max_colwidth #433

Closed
wants to merge 1 commit into from

Conversation

AVHopp
Copy link
Collaborator

@AVHopp AVHopp commented Nov 22, 2024

This PR fixes #432 by reducing the max_colwidth argument when pretty printing dataframes from 16 to 14.

The result can be seen on my fork: https://avhopp.github.io/baybe_dev/latest/examples/Serialization/basic_serialization.html

@AVHopp AVHopp added the documentation Improvements or additions to documentation label Nov 22, 2024
@AVHopp AVHopp self-assigned this Nov 22, 2024
Copy link
Collaborator

@AdrianSosic AdrianSosic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @AVHopp 👋🏼 To be honest, this doesn't seem the right fix for me. Two reasons:

  1. Changing the global default to fix one single "local" problem seems weird to me. After all, the having the max_colwidth configurable as an argument means we can control the width locally wherever we want. So changing the default is like saying "I have a controllable argument x and use the corresponding function in many places, but instead of passing the appropriate local inputs, I'll always adjust the default to the best possible common denominator". So what is the purpose of having it controllable then in the first place? I think default should be set to some value that is "generally good" and exceptions should be overriden locally.
  2. More importantly: this only fixes symptoms, not the actual cause. Do we have any idea what actually causes the problem? I suspect: there are additional line wrapping steps in your doc script that cause the problem. If this is the case, then we'd simply run into the same problem again once we change the example such that it gets one additional column / or in other examples.

Can we rather try to pinpoint the problem itself?

Copy link
Collaborator Author

@AVHopp AVHopp Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answering @AdrianSosic here to create a thread.
As far as I know, the problem is that too long lines just cause weird line breaks in our documentation. You can see that in multiple parts, also in headings.

I see the following ways of avoiding this (other than what we do here):

  1. Manual manipulation of md/html files (which is something you did not want previously and which is very ugly to do, while it is not even clear if this works)
  2. Redesigning experiments/examples such that the lines are not too long.
  3. Don't explicitly print full objects in the examples (this would probably be the safest, I think that there are several places where we print a whole campaign although the search space would be sufficient)
  4. Weird sphinx/myst magic that might or might not work

Also, before claimingf "I think there is some weird line wrapping" please provide actual evidence of that happening.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The easiest way to fix this exact issue would probably be to just reduce the names of the parameters in this example. For example, I do not think that using the following parameter names instead of the current ones would hurt in any way:

parameters = [
    CategoricalParameter(
        name="Granularity",
        values=["coarse", "medium", "fine"],
        encoding="OHE",
    ),
    NumericalDiscreteParameter(
        name="Pressure",
        values=[1, 5, 10],
        tolerance=0.2,
    ),
    NumericalDiscreteParameter(
        name="Temp",
        values=np.linspace(100, 200, 10),
    ),
]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing locally, this indeed solves the problem in the most elegant way imo.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that also seems like just treating the symptom
why are long lines creating line breaks that break the formatting?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, again symptoms. I think chances are high that this is caused by the textwrap.wrap call in the script, which probably gets applied in some context where it breaks the markdown

@AVHopp AVHopp marked this pull request as draft November 26, 2024 12:48
@AVHopp
Copy link
Collaborator Author

AVHopp commented Nov 26, 2024

Closed since this is now part of the general issue regarding documentation and we agreed to not implement a hotfix for this but rather investigate the root issue.

@AVHopp AVHopp closed this Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken Docs of Serialization
3 participants