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

Adjust the boundary plotting #338

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Gene0315
Copy link
Collaborator

@Gene0315 Gene0315 commented Jun 1, 2024

Following the issue #335
The PR sets the value at boundary point equal to the inside one.

Features: The plotting looks more reasonable than the original one.
Strength: The adjustment is simple and makes the result look nicer.
Weakness: This way is too artificial and simplified and lack of physical basis. The result shows that there is still something wrong on the boundary, especially the left boundary.

截圖 2024-06-01 下午2 42 33

Copy link
Member

@yungyuc yungyuc left a comment

Choose a reason for hiding this comment

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

Thanks, @Gene0315 . This is a good starting point. I agree this is a very each patch but it may be too easy to be useful. The modification changes the outside points in a purely artificial way and one cannot predict the approach based on the understandings of the CESE method nor the boundary-condition treatment. It makes it hard to maintain (understand).

This modification changes the solution array instead of changing the way we plot the array. A more reasonable way is to just change the plotting code.

There may be follow-up work for enhancing the solution array size once the plotting is made more beautiful.

@yungyuc
Copy link
Member

yungyuc commented Jun 1, 2024

@j8xixo12 comments?

@j8xixo12
Copy link
Collaborator

j8xixo12 commented Jun 1, 2024

@j8xixo12 comments?

I agree with @yungyuc viewpoint, we should modify the plotting code not the solution data, because modifying solution data may effect the computation result, but plotting code not.
I think we can work together to enhance the plotting code !

@Gene0315
Copy link
Collaborator Author

Gene0315 commented Jun 1, 2024

@j8xixo12 comments?

I agree with @yungyuc viewpoint, we should modify the plotting code not the solution data, because modifying solution data may effect the computation result, but plotting code not. I think we can work together to enhance the plotting code !

Ok, I know what I did improperly. I will try to revise the code in plotting section without changing the solution data.
The first thing I plan to do is to remove the boundary point on the figure.

@yungyuc yungyuc added viewer Visualize stuff onedim One-dimensional solver and removed drawer labels Jun 2, 2024
@yungyuc
Copy link
Member

yungyuc commented Jun 2, 2024

That sounds like a good next step, @Gene0315 . When do you plan to be ready for the next review?

When you are ready, please annotate the important lines like what was done in #337 (comment) and other PRs. And also leave a global comment explicitly saying that you are ready for the next review. If anything is not clear to you, go to the discord channel to ask for help.

@Gene0315
Copy link
Collaborator Author

Gene0315 commented Jun 4, 2024

@yungyuc
I think I can finish it this Friday or Saturday.

@yungyuc yungyuc added the enhancement New feature or request label Jun 9, 2024
@yungyuc
Copy link
Member

yungyuc commented Aug 25, 2024

@yungyuc I think I can finish it this Friday or Saturday.

@Gene0315 We forgot to track this. Any update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request onedim One-dimensional solver viewer Visualize stuff
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants