functions- multi parameters lesson needs to be cleaned up

[lwasser]

I added this lesson because I thought it could add to our existing functions lesson. But some of the content (maybe all) might be too dated. so it would be valuable for someone to read through the lesson and decide if we want to keep it in our batch of function lessons!

We could also remove it entirely for now - but i added it to avoid toctree warnings

[sneakers-the-rat]

well first of all it looks like this to me

[sneakers-the-rat]

looks like myst wants to end the previous section because the header element starts a new one, and that catches the opening <div>, but then the closing </div> only has the whole body div to close

[sneakers-the-rat]

this is a sort of old bug that happens a lot with myst, and i spent half an hour trying to remember how i've resolved it in the past, but then i realized that there isn't any css applied to the notice--warning class anyway, so if you just take out those divs it shoudl be good :).

Another way of applying styles like that would be to class the warning admonition, or use raw directives to wrap the section. it's just a really annoying bug in how you need a line break between the markdown and the div wrapper and how myst parses sections.

[willingc]

I learned when doing collapsible admonition blocks in #59 that some admonitions only use html+css where others need JS in the browser: https://jupyterbook.org/en/stable/content/components.html#dropdowns See important note at end of sections on Dropdown and Dropdown Admonitions.

[sneakers-the-rat]

I SWEAR i tried to do a div directive earlier and it didnt work, i must have been hallucinating.

[willingc]

@lwasser @sneakers-the-rat I'm going to do a quick editing pass on the content of this lesson.

[lwasser]

Sounds great @willingc thank you!! I'm spending a bit of time on participant communication right now.

[sneakers-the-rat]

oop you were too fast for me and merged #67 as i was writing a comment (edit: and then i got pulled away to something for a minute), hopefully it's not rude to reopen this:

the part of the lesson re: making a mean_mm_to_in seems odd to me. It strikes me as a bit of an antipattern? I get that the idea is to introduce optional parameters via axis_value but it also demos overloading functionality in a single function rather than composing functions.

e.g. outside of the context of a lesson I would encourage someone to split that function up and not wrap the function like

def mm_to_in(mm: float) -> float:
    return mm / 25.4

data = np.random.default_rng().random((10,10))
value = mm_to_in(np.mean(data))

similarly i get the idea of the lesson re: conditionals and checking for None, but in a code review I would propose refactoring that to

def mean_mm_to_in(data_mm: float, axis_value=None) -> float:
    mean_data = np.mean(data_mm, axis=axis_value)
    # ...

# or 

def mean_mm_to_in(data_mm: float, **kwargs) -> float:
    mean_data = np.mean(data_mm, **kwargs)
    # ...

since axis=None is the same thing as not passing the kwarg at all.

Maybe another example is

def mm_to_imperial(mm: float, unit: Literal['in', 'ft'] = 'in') -> float:
    if unit == 'in':
        return mm_to_in(mm)
    elif unit == 'ft':
        return mm_to_ft(mm)
    else:
        raise ValueError(f"Unit must be one of ['in', 'ft'], got: {unit}")

to demo function composing, optional params, conditionals, and that gets us another exception example too?

I am really really not trying to be critical and also get that these are toy examples meant to demonstrate basic syntax, but i think to the degree we can be demoing good practices in the code we should? and I think "overloading a function to do multiple things" is a super common early programmer pattern that is helped a lot by showing how to compose functions.

also not asking you to rewrite a ton of stuff, if that sounds reasonable i would contribute edits, or else just ignore and re-close and don't worry about it i won't be offended.

[willingc]

@sneakers-the-rat I was thinking something similar as I was going through it. I cleaned up the naming slightly. I too would have called one function out to another.

The whole lesson is built around the current example. Perhaps a box at the end that talks about composing functions and calling out to other functions (i.e. an alternative and likely better practice, but perhaps more advanced than the focus on multiple parameters).

[willingc]

Also, we could use a docstring to explain and perhaps even put in the docstring. "Let's consider refactoring into two functions in the future." That's an approach that would be done in the real world. Document until you can refactor. 😄

[willingc]

@sneakers-the-rat I feel an advanced functions lesson in our future 😉

[willingc]

Opened #69 to capture future ideas.

[willingc]

@sneakers-the-rat Are you good with reclosing this and moving discussion to #69? If there are specific items prior to fall festival that need to be addressed, let's list them here. I'm trying to use the "fall festival" milestone to track our collective to do list.

[sneakers-the-rat]

sure sure, here i'll pr with the aside admonition