Skip to content

Conversation

@YichengDWu
Copy link

No description provided.

@YichengDWu
Copy link
Author

BTW, is axies a typo?

@ChrisRackauckas
Copy link
Member

Looks like it. It's probably worth fixing. Though it would be good to choose a name that doesn't cause shadowing.

@YichengDWu
Copy link
Author

What drove me to write these docstrings was that I found it hard to understand the source code of MOL. With these at least I can understand the most part of http://methodoflines.sciml.ai/dev/devnotes/.

@codecov
Copy link

codecov bot commented Jul 9, 2022

Codecov Report

Merging #125 (6eb87da) into master (ae0bd89) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #125   +/-   ##
=======================================
  Coverage   88.29%   88.29%           
=======================================
  Files          13       13           
  Lines         940      940           
=======================================
  Hits          830      830           
  Misses        110      110           
Impacted Files Coverage Δ
src/MOLFiniteDifference.jl 87.50% <ø> (ø)
src/MOL_utils.jl 84.12% <100.00%> (ø)
src/discretization/discretize_vars.jl 86.41% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ChrisRackauckas ChrisRackauckas merged commit c744e7c into SciML:master Jul 9, 2022
@YichengDWu
Copy link
Author

There's a lot of naming worth fixing...

@ChrisRackauckas
Copy link
Member

That's always easy to do right before dropping the next major (or here, v1.0), so I never tend to worry about it until there's a sense they will lock in. But if you wish to correct a few, please don't shy away.

@xtalax
Copy link
Member

xtalax commented Jul 10, 2022

Axies was originally a typo, I decided to leave it to avoid shadowing but am open to other ideas. As for a lot of the shorter more euphemistic names like s x\bar u\bar and x2i having them longer really interferes with readability in a lot of the longer statements since they are used so frequently.

@xtalax
Copy link
Member

xtalax commented Jul 10, 2022

Thank you for this!

@YichengDWu
Copy link
Author

We have talked about this. Annotating the type of s is a very good start. I'm not saying that variable names should simply be longer. But they should meet the standards of clean code. Like changing params to spvars or get_spvars would be much better. Also sometimes the discretization is very slow and there seems to be a lot of room for performance improvement

@ChrisRackauckas
Copy link
Member

Also sometimes the discretization is very slow and there seems to be a lot of room for performance improvement

Right now the code size is O(1/dx), i.e. if you have more discretization points then you have a larger code size. That is due to the scalarization in the IR. It needs to change to O(1) before we can call the library complete, and that's what the stencil forms and symbolic IR stuff is all about.

@xtalax
Copy link
Member

xtalax commented Jul 12, 2022

I find spvars quite undescriptive, this is params to reference how they are generated i.e. with @parameters. I would support changing this to get_params however. I would argue that for code as obfuscated as this readability is of the highest priority, so if you would like to try to annotate any types with their semantic meaning, we can have a back and forth on which names may be better.

In other news I am starting work on integrating the new stencil interfaces. This is a big job so will attempt this in stages, I would really appreciate your help if you want to help with this.

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.

3 participants