search

dhruvbhagtani / toy-model

This is a hybrid model of the global ocean with simplified governing equations to understand large scale circulations.
4 stars 0 forks source link

Let's start submitting PRs for changing code? #10

Closed navidcy closed 3 years ago

navidcy commented 3 years ago

@dhruvbhagtani2105, I saw you pushed the Euler module. I have some remarks on that. I'll raise an issue.

But let's start submitting PRs for modifying the code. That way we'll all have the chance to comment/review code before it's merge in the repository. It's anyway a good practice. ;)

dhruvbhagtani commented 3 years ago

Sure @navidcy. Happy to hear your thoughts.

navidcy commented 3 years ago

Sure @navidcy. Happy to hear your thoughts.

I'm afraid I'm being repetitive here :(, but the gist of my comments is that the module is neither commented, nor docstring, nor tested. Perhaps we should revisit it after we discuss the assignment on Fri?

dhruvbhagtani commented 3 years ago

Hi Navid - sorry for replying so late. I just finished commenting the 1D_diffusion.ipynb, Periodic_boundary_conditions.ipynb and the Euler module (this one partially). Each of them have been commented to some extent, and I've added docstrings to each function for easier readability.

navidcy commented 3 years ago

I'll go through it today tomorrow.

What happened with https://github.com/dhruvbhagtani2105/toy-model/issues/10? :)

dhruvbhagtani commented 3 years ago

Ah, I forgot. I've created a new branch - Dhruv_codes.

navidcy commented 3 years ago

So when you feel you have something ready to merge to main submit a PR? Or should I have a look at the branch?

navidcy commented 3 years ago

I'll close this issue since we are bypassing it. :)