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

Implement two-output division #89

Merged
merged 15 commits into from Mar 7, 2023
Merged

Implement two-output division #89

merged 15 commits into from Mar 7, 2023

Conversation

Chris00
Copy link
Contributor

@Chris00 Chris00 commented Mar 2, 2023

This implements two-output division as discussed in #87

@Chris00 Chris00 force-pushed the master branch 2 times, most recently from 1638495 to 0ff1ed7 Compare March 2, 2023 11:37
unageek added a commit to unageek/ITF1788 that referenced this pull request Mar 4, 2023
@unageek
Copy link
Owner

unageek commented Mar 4, 2023

Hi! Thank you for the PR! Could you generate tests for mul_rev_to_pair following the instructions below?

  1. pushd ITF1788
  2. git fetch upstream (whatever you named https://github.com/unageek/ITF1788)
  3. git checkout inari-pr-89
  4. popd
  5. ./tools/gen_itf1788_tests.sh
  6. cargo fmt
  7. Uncomment the next line:
    //mod libieeep1788_mul_rev;

@unageek unageek added the enhancement New feature or request label Mar 4, 2023
@Chris00
Copy link
Contributor Author

Chris00 commented Mar 4, 2023

I've done so.

@Chris00
Copy link
Contributor Author

Chris00 commented Mar 4, 2023

Rebased.

@unageek
Copy link
Owner

unageek commented Mar 4, 2023

Could you revert the change to .github/workflows/build.yml?

@coveralls
Copy link

coveralls commented Mar 4, 2023

Pull Request Test Coverage Report for Build 4352295643

  • 124 of 124 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 85.212%

Totals Coverage Status
Change from base Build 4331625234: 0.3%
Covered Lines: 2172
Relevant Lines: 2213

💛 - Coveralls

@Chris00
Copy link
Contributor Author

Chris00 commented Mar 4, 2023

Could you revert the change to .github/workflows/build.yml?

I deleted the relevant commit.

src/basic.rs Outdated Show resolved Hide resolved
src/basic.rs Outdated Show resolved Hide resolved
src/basic.rs Outdated Show resolved Hide resolved
src/basic.rs Outdated Show resolved Hide resolved
src/basic.rs Outdated Show resolved Hide resolved
src/basic.rs Outdated Show resolved Hide resolved
@Chris00
Copy link
Contributor Author

Chris00 commented Mar 4, 2023

I performed the requested changes. Did not find how to test the KaTeX doc however.

@Chris00 Chris00 requested a review from unageek March 4, 2023 17:52
@unageek
Copy link
Owner

unageek commented Mar 5, 2023

You can build the documentation with:

RUSTDOCFLAGS="--cfg docsrs --html-in-header /path/to/inari/src/_docs/header.html" cargo doc --open

Note that the path to header.html must be absolute.

@unageek
Copy link
Owner

unageek commented Mar 5, 2023

By the way, do you have any ideas about how the decorated version of mul_rev_to_pair might be used?

@Chris00
Copy link
Contributor Author

Chris00 commented Mar 5, 2023

You can build the documentation with:

Thanks — I should have reread the README! (I made a shell script to stop forgetting. 🤞)

do you have any ideas about how the decorated version of mul_rev_to_pair might be used?

No. Actually, books and papers in my field (computer assisted proofs) seldom mention decorated intervals. Maybe in constraint-satisfaction problems but I am less familiar with that.

@unageek
Copy link
Owner

unageek commented Mar 6, 2023

Could you enhance the documentation in two points?

  • Emphasize the difference between division and reverse multiplication.
    • Users may take the word two-output division literally. (It might be better to avoid that expression?)
  • Explain how the decorated version of the function is defined.

@Chris00
Copy link
Contributor Author

Chris00 commented Mar 6, 2023

I've done so. Let me know what you think.

Copy link
Owner

@unageek unageek left a comment

Choose a reason for hiding this comment

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

Thank you so much! Please check some more comments.

src/basic.rs Outdated Show resolved Hide resolved
src/basic.rs Show resolved Hide resolved
src/basic.rs Show resolved Hide resolved
src/basic.rs Outdated Show resolved Hide resolved
src/basic.rs Show resolved Hide resolved
src/basic.rs Show resolved Hide resolved
@unageek
Copy link
Owner

unageek commented Mar 7, 2023

GAOL uses the term relational division. I love this one! I'm not going to use it in the crate documentation, though.

@unageek
Copy link
Owner

unageek commented Mar 7, 2023

LGTM! Thank you!

@unageek unageek merged commit 0bcfe93 into unageek:main Mar 7, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants