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

feat: Add derivatives, incl. high-order and mixed partial, using macros #3703

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Leedehai
Copy link

@Leedehai Leedehai commented Sep 3, 2022

Please note this is a reimplementation (motivated by #3701 (comment)) of my previous PR #3701, with feature parity.

Support ordinary derivatives \odv and partial derivatives \pdv.

It supports high-order mixed partial derivatives. The syntax emulates popular $\LaTeX$ packages that provide this sought-after feature.

  • Demo. Also in test screenshots.
  • 'yarn start' URL that produced the demo.
  • Sensible fallbacks; if users mistyped their expressions they wouldn't be confused by blankness on the screen.
  • Produced screenshots for tests.
  • Added to the docs.
  • Already signed CLA.

@Leedehai
Copy link
Author

Leedehai commented Sep 3, 2022

Hi maintainers, GitHub says I "need a maintainer to approve running workflows".

I reimplemented my own PR #3701 per discussions in issues #3038 and #3184.

@Leedehai Leedehai changed the title feat: Add derivatives, incl. high-order and mixed partial, with macros feat: Add derivatives, incl. high-order and mixed partial, using macros Sep 3, 2022
@Leedehai
Copy link
Author

Leedehai commented Sep 5, 2022

Looking at the failed screenshotter test on Safari (link) it seems like a fleeting issue in the testing infra, not the code: RequestError: socket hang up. Re-running the test might be enough to fix it, but I'm not able to trigger a re-run here on GitHub.

yarn test:screenshots was passing locally using Docker images.

@Leedehai
Copy link
Author

Leedehai commented Sep 5, 2022

Just managed to re-trigger a test rerun by pushing an empty commit, and the screenshotter test passed on all three browsers (chrome, firefox, safari). Problem solved :)

That said, some GitHub workflows are still pending since GitHub still says I "need a maintainer to approve running workflows".

@codecov
Copy link

codecov bot commented Sep 5, 2022

Codecov Report

Merging #3703 (d3743c8) into main (176552a) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3703      +/-   ##
==========================================
+ Coverage   92.97%   93.04%   +0.06%     
==========================================
  Files          91       92       +1     
  Lines        6765     6826      +61     
  Branches     1571     1586      +15     
==========================================
+ Hits         6290     6351      +61     
  Misses        437      437              
  Partials       38       38              
Impacted Files Coverage Δ
src/macros.js 97.53% <100.00%> (+0.03%) ⬆️
src/macros/derivatives.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef49e2b...d3743c8. Read the comment docs.

@Leedehai
Copy link
Author

Ping reviewers.. 🙂

@Leedehai
Copy link
Author

Leedehai commented Sep 20, 2022

Ping reviewers..🙂 (2nd week)

@Leedehai
Copy link
Author

Leedehai commented Sep 27, 2022

Ping reviewers..🙂 (3rd week)

@Leedehai
Copy link
Author

Leedehai commented Oct 5, 2022

Ping reviewers...🙂 (4th week)

@Leedehai
Copy link
Author

Leedehai commented Oct 11, 2022

Ping reviewers...🙂 (5th week)

@Leedehai
Copy link
Author

Leedehai commented Oct 26, 2022

Ping reviewers... (7th week)

@ronkok @edemaine @ylemkimon @ry-randall

Copy link
Member

@edemaine edemaine left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. (And it'd be better to limit the number of pings you do.) I had some time today to do a high-level review, and have a few comments that we should discuss before a more thorough review.

## Derivatives

The syntax closely emulates popular $LaTeX$ packages that provide derivatives,
such as `physics`, `diffcoef`, and `derivative`.
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather see this follow one spec. Or perhaps it already does, and this sentence just needs rewording? Fine to link to other packages, but which one should be considered "correct" for this PR? We don't want to implement a brand new derivative behavior. Stated another way, if someone was to copy KaTeX code over to LaTeX, which \usepackage should they include? (This will also help review this PR; I'd rather not read the documentation of three different packages, though I've already spent some time skimming them all.)

Copy link
Author

@Leedehai Leedehai Oct 27, 2022

Choose a reason for hiding this comment

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

Yes, this follows one spec. Let me reword..

return {func, vars, totalOrders, varOrders};
}

const PLACEHOLDER = "\\square";
Copy link
Member

Choose a reason for hiding this comment

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

Is this placeholder behavior implemented in any of the existing LaTeX packages? I didn't see it anywhere, but I may have missed it in one of them.

Copy link
Author

Choose a reason for hiding this comment

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

No it's not implemented in any existing LaTeX package. That said, I think it's a useful improvement, so that users won't be puzzled by blankness.

Copy link

Choose a reason for hiding this comment

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

The square is a meaningful operator in many contexts and that might confuse more than it helps. I suppose if someone leaves the [options] blank they should expect that location in fact to be blank.

"\\odv", (context) => assembleDerivativeExpr("\\textnormal{d}", context));
defineMacro(
"\\pdv", (context) => assembleDerivativeExpr("\\partial", context));

Copy link
Member

Choose a reason for hiding this comment

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

I think it would make more sense to move these side-effects into src/macros/derivatives.js, which would then import defineMacro from "./defineMacro".

However, we haven't decided to make a src/macros directory. Perhaps you could provide your input on #2994? What do you think of putting your code in src/functions/derivatives.js, and possibly renaming functions to commands?

@visika
Copy link

visika commented Jan 12, 2023

I'm very excited to see this implemented, and I can't wait to use that! Is there an estimate on the timeline to release that?

@Leedehai
Copy link
Author

Leedehai commented Jan 15, 2023

@visika Hi I lost access to my Linux cloud machine a while ago but I can't get Dorker (needed for tests) working properly on my mac.

Would love you -- or anyone else interested -- to take over this PR. 🙂

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

3 participants