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

Add numerical support of other real types (helper) #1948

Closed
wants to merge 9 commits into from

Conversation

huiyuxie
Copy link
Member

@huiyuxie huiyuxie commented May 17, 2024

Extra part of #1909. Better saved as draft as it may take longer time.

Tasks:

  • auxiliary/math - check todo
  • surface_flux_function

@huiyuxie huiyuxie requested a review from ranocha May 17, 2024 21:44
Copy link
Contributor

Review checklist

This 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

  • The PR has a single goal that is clear from the PR title and/or description.
  • All code changes represent a single set of modifications that logically belong together.
  • No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

  • The code can be understood easily.
  • Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
  • There are no redundancies that can be removed by simple modularization/refactoring.
  • There are no leftover debug statements or commented code sections.
  • The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

  • New functions and types are documented with a docstring or top-level comment.
  • Relevant publications are referenced in docstrings (see example for formatting).
  • Inline comments are used to document longer or unusual code sections.
  • Comments describe intent ("why?") and not just functionality ("what?").
  • If the PR introduces a significant change or new feature, it is documented in NEWS.md with its PR number.

Testing

  • The PR passes all tests.
  • New or modified lines of code are covered by tests.
  • New or modified tests run in less then 10 seconds.

Performance

  • There are no type instabilities or memory allocations in performance-critical parts.
  • If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

  • The correctness of the code was verified using appropriate tests.
  • If new equations/methods are added, a convergence test has been run and the results
    are posted in the PR.

Created with ❤️ by the Trixi.jl community.

@huiyuxie huiyuxie marked this pull request as draft May 17, 2024 21:44
@huiyuxie huiyuxie self-assigned this May 17, 2024
@huiyuxie
Copy link
Member Author

Redo #1929 to keep consistency.

I may start my project soon so the whole type issue here may slow down (but won't stop). Let me know if you consider this urgent, and I may adjust my priority :)

Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

Thanks! For now, I suggest making the TODO notes more concrete. Besides that, this is open-source software, so we're happy about all contributions and know that other things are happening in real life.

src/auxiliary/math.jl Outdated Show resolved Hide resolved
src/auxiliary/math.jl Outdated Show resolved Hide resolved
@huiyuxie
Copy link
Member Author

What do you mean by other things happening in real life?

Copy link

codecov bot commented May 20, 2024

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 96.12%. Comparing base (b98b6f0) to head (d9e0f50).
Report is 95 commits behind head on main.

Files Patch % Lines
src/auxiliary/math.jl 0.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1948      +/-   ##
==========================================
- Coverage   96.15%   96.12%   -0.03%     
==========================================
  Files         454      454              
  Lines       36509    36521      +12     
==========================================
  Hits        35103    35103              
- Misses       1406     1418      +12     
Flag Coverage Δ
unittests 96.12% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ranocha
Copy link
Member

ranocha commented May 21, 2024

I can't spend as much time on this open-source project as I would like since I also have to other things

@huiyuxie
Copy link
Member Author

I thought it means something more abstract and high level, but if you are talking about this, no worries.

Does that mean I have total control over my project? As I was always thinking about making this a totally separate package within the ecosystem, like https://github.com/SciML/DiffEqGPU.jl, instead of integrating it directly into the original package.

At least keep an eye on my project - sometimes it moves fast.

@ranocha
Copy link
Member

ranocha commented May 21, 2024

Does that mean I have total control over my project? As I was always thinking about making this a totally separate package within the ecosystem, like https://github.com/SciML/DiffEqGPU.jl, instead of integrating it directly into the original package.

This is open-source software, so we can not disallow this (as long as you respect the license). We would, of course, be happy if you continued contributing to the main Trixi.jl repo - in particular with fixes for floating point types like in this PR.

@huiyuxie
Copy link
Member Author

huiyuxie commented May 21, 2024

I will try my best as I work full time during daytime.

This is open-source software, so we can not disallow this (as long as you respect the license).

Why not say, "This is open-source software, so you are allowed to do so"? You are saying something that seems exclusive to me. I believe everyone comes to open-source out of interest and all kinds of contribution are encouraged. This is just so wired for me, I just don’t get the same feeling from other places...

Sorry for above if you are not feeling well, but I am also not feeling well 🥲

@huiyuxie
Copy link
Member Author

huiyuxie commented May 22, 2024

This is also offensive. This implies that you already consider the possibility that I may not respect the license. Would you say something like this to your colleagues? Probably no. I'm feeling a sense of inequality and hierarchy from you. Please mind your words even if you don't mean that way. I need some time to adjust myself - sorry for that

I clearly know that MIT license is somehow the most flexible one - for me, only a copycat would be likely to violate it.

@huiyuxie
Copy link
Member Author

huiyuxie commented May 22, 2024

I seldom feel a sense of inclusion here. Decisions are made by people who are not actually doing the work, instead of through an open and transparent vote. I feel like I’m just doing tasks that are assigned to me, and independent thinking and open-mindedness are discouraged. Maybe I chose the wrong place - this is an open-source place but makes me feel like a school, some people (includes me) are even tagged with the label of student

@ranocha
Copy link
Member

ranocha commented May 26, 2024

I'm sorry if my words above sound aggressive to you - that was not my goal. Please do whatever you like to do and work on the open-source projects that you enjoy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants