search

vega / altair

Declarative statistical visualization library for Python
https://altair-viz.github.io/
BSD 3-Clause "New" or "Revised" License
9.4k stars 795 forks source link

docs: Promote `when-then-otherwise` in User Guide #3544

Closed dangotbanned closed 3 weeks ago

dangotbanned commented 3 months ago

Will close #3500

Status

All outstanding work should be scoped to parameters.rst. Corrections/suggestions elsewhere are still welcome

The linked version represents Draft 1.

Tasks

dangotbanned commented 2 months ago

@joelostblom since your mention of this PR in https://github.com/vega/altair/discussions/3548#discussioncomment-10536857, I've circled back to this.

If you have the time, would you be able to double-check the tasks I've added in the description please?

Going to start working through them now

joelostblom commented 2 months ago

Looks great! The location for the callout seems suitable to me. The only thing I would add is that I think we should include an example in the user guide section for the use of empty inside the conditional, as opposed to in the parameter definition (https://github.com/vega/altair/pull/3490).

dangotbanned commented 2 months ago

Looks great! The location for the callout seems suitable to me. The only thing I would add is that I think we should include an example in the user guide section for the use of empty inside the conditional, as opposed to in the parameter definition (#3490).

Perfect thanks @joelostblom, I've added that to the list.

Managed to get through a good chunk today, I'm expecting parameters.rst will require the most work since it calls out alt.condition directly

joelostblom commented 2 months ago

@dangotbanned I noticed that you tagged me for review here, but also that the PR is still marked as draft, so I just want to double check whether you want a review now or you are waiting to finish something up first.

dangotbanned commented 2 months ago

@dangotbanned I noticed that you tagged me for review here, but also that the PR is still marked as draft, so I just want to double check whether you want a review now or you are waiting to finish something up first.

Thanks for checking in @joelostblom!

I was hoping to get feedback focused on the Status section of the description.

Do you think I should move this out of draft - to make it clearer that I'm interested in a review?

dangotbanned commented 2 months ago

Thanks for your patience @dangotbanned and sorry for not getting here earlier. I had a quick read in the docs and think this is looking great already!

No worries @joelostblom, thanks for taking the time to review

I think it would be instructor to include the following examples in a subsection at the end of "Conditions and Filters":

  • Simple conditions may be expressed without defining a default (maybe with a simpler example than invalid=None)
  • Chain calls to express precise queries
  • Reuse defined when variable

Sounds good to me. Just to be sure I've understood correctly, which of these locations did you mean?

  1. End of Conditional Encodings
    • The end of the alt.when section of Conditions & Filters
  2. End of Conditions & Filters
    • The actual end of Conditions & Filters

Whereas the following seem more like "another way of doing something we already have shown" and I would leave them out of the User Guide (maybe a link a the bottom of the new subsection mentioning there are even more example in the when API reference?)

  • Predicates passed as positional arguments will be reduced with & (since this is just an alternative way of achieving something)
  • Using keyword-argument constraints can simplify compositions (since this is only for AND and ==)

That makes sense, I definitely want to avoid adding too much here - especially if as you've said there is already a documented way to do it

joelostblom commented 2 months ago

While reviewing https://github.com/vega/altair/pull/3544/commits/50a520e9314108d572ef8bd6d48f0135151d2c6c, I got some ideas for rearranging sections on this page to make it easier to follow. I have most of it done and will finish it off tomorrow to show you my suggestion instead of trying to explain it.

dangotbanned commented 2 months ago

While reviewing 50a520e, I got some ideas for rearranging sections on this page to make it easier to follow. I have most of it done and will finish it off tomorrow to show you my suggestion instead of trying to explain it.

All good with me @joelostblom

Feel free to move this PR out of draft when you're ready. I was waiting until either I came up with a heading for Untitled More When or you took a look

dangotbanned commented 1 month ago

Thanks for your helpful feedback on the sections I edited @dangotbanned and sorry it has taken me so long to get back to this. Work has gotten the best of me lately =/ I believe I addressed all your comments so we should be good to go for that section. In terms of reviewing the rest of the examples you have changed, I looked through some of the ones where there is only a few lines changed and they seem good (but I will go through more thoroughly, hopefully tomorrow).

No problem @joelostblom, thanks for updating.

All looking good to me now, I'd hit approve now if I wasn't the author 😄

dangotbanned commented 1 month ago

As for the larger changes where the formatting of the code has also changed, do you want to change to the syntax we are using currently in the docs or push forward the discussion in #3570 for potentially changing the default syntax? I would like to avoid that we have a mix of the two, but don't feel too strongly for which one we pick. I forget if you had joined the call when we talked briefly about this and there was a couple of points brought up regarding keeping the old syntax because it might be more similar to how new people write who are not familiar with autoformatters and thus easier for them, but nothing is set in stone I believe.

Ah yeah it would seem I missed that part of the call, apologies @joelostblom.

If there were any changes in positions/consensus then updating #3570 would be helpful for visibility.

Regarding this PR (and reiterating https://github.com/vega/altair/pull/3544#discussion_r1746899291) I'm happy for you to make any formatting changes that you think would help readability 👍

dangotbanned commented 4 weeks ago

@joelostblom I wanted to check in with you, as I'm hoping to merge this PR soon.

The following are complicated somewhat by this remaining open:

I'm thinking pragmatically here, if the only possibly outstanding concerns were regarding formatting - would you be okay if we tackled that in a follow up?

I'm really happy with these updates we put together and wish to build on them further in the other issues/PRs I linked

dangotbanned commented 3 weeks ago

@mattijn if you're available

I'm planning to merge this tomorrow and follow it up with another PR #3668 mentioned in https://github.com/vega/altair/pull/3664#discussion_r1824434520

The short rundown is that it will be adding support all the classes like this:

when used in both of these calls:

I can delay this if anyone would have an issue - but as I mentioned in https://github.com/vega/altair/pull/3544#issuecomment-2445055293 - I'd really like to close this PR off if possible

mattijn commented 3 weeks ago

I haven't had a look to this PR before, but just had a quick glance. I understand the title to promote the when-then-otherwise in the user guide, but (unintentionally?) you ruffed many examples into another style that we did not like to promote in the examples.

mattijn commented 3 weeks ago

During NumFOCUS summit @binste and I had come up with a config object based on YAPF that comes close to the style of the Altair examples how one would write it manually.

One can test that config style as such:

import yapf
from yapf.yapflib.yapf_api import FormatCode

# Define the code you want to format
code = """
def example_function(x,y):
 return (x+y)* (x-y)
"""

# Define YAPF configuration
style_config = {
    'based_on_style': 'pep8',
    'COLUMN_LIMIT': 88,
    'SPLIT_BEFORE_EXPRESSION_AFTER_OPENING_PAREN': True,
    'SPLIT_BEFORE_FIRST_ARGUMENT': True,
    'ALLOW_SPLIT_BEFORE_DEFAULT_OR_NAMED_ASSIGNS': True,
    'NO_SPACES_AROUND_SELECTED_BINARY_OPERATORS': False,
    'DEDENT_CLOSING_BRACKETS': True,
    'COALESCE_BRACKETS': True,
    'SPLIT_BEFORE_DOT': True,
    'SPLIT_BEFORE_CLOSING_BRACKET': True,
}

# Format the code using YAPF
formatted_code, _ = FormatCode(code, style_config=style_config)

# Display formatted code
print(formatted_code)

I've to mention that this style is not endorsed by @binste, he just prefer to ruff it all! 😄

joelostblom commented 3 weeks ago

@dangotbanned Sorry for the delay here. I wanted to go in an revert the examples to the old syntax, but I've been buried in work lately =/ I don't want to hold up your future PRs, so I'm ok if this needs to be merged although if there is something that can be done to bring it closer to the old syntax (like what Mattijn is suggesting maybe?) then that might save time long term instead of trying to change these back in a follow up PR.

As I mentioned in https://github.com/vega/altair/discussions/3570 I do see the advantages of adhering to a syntax that is supported by Ruff, and if our current syntax is making it hard to update the docs effectively, then that's another argument in favor of switching.

joelostblom commented 3 weeks ago

Thank you for going through and reverting the style on all of these examples before merging @dangotbanned !