-
Notifications
You must be signed in to change notification settings - Fork 112
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
Provide document of numeric types and type stability #1938
Conversation
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
Float32
)Float64
and Float32
)
Float64
and Float32
)
Please check again now :) First, please check style, I try to keep consistency with the rest of the convention file. Second, concerning content, I think handling these cases on a case-by-case basis is acceptable for now (but I have no idea if this is good for other developers). Third, I suggest asking someone unfamiliar with these things to review it again. While you understand it clearly, it may not be as easy for others to grasp. |
Shall we merge this PR if it looks good to you, or should it be held until the main PR is ready to merge? I think I should check the main PR again based on your provided notes. Please wait |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks good to me. It would be nice to get feedback from @sloede next. Then, we can merge this PR and continue with the other ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for adding this to the docs! I only have some small suggestions for adding additional context, plus some formatting changes (I'd use proper subheadings such that we can directly link to them)
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bearing with me @huiyuxie! I've made a new suggestion, where I leave your original part about exact FP values virtually unchanged, but add some clarification later.
Once these final remarks are resolved, this is ready to merge 😊
Co-authored-by: Michael Schlottke-Lakemper <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thanks, these are good suggestions! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Covers #1909. It is just a start and later I will add more documentation.