-
-
Notifications
You must be signed in to change notification settings - Fork 613
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
Docstrings for f32 and f64 #1549
Conversation
Inserting docstrings for methods `f32()` and `f64()`.
Great job! Nearly there. I would probably say something like converts the parameters of the model to the corresponding datatype. |
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.
Looks great! I left some suggestions inline including Dhairya's changes.
Now that you added the docstring, you need to add it to the docs so that the new docstring is included. This would probably be a small section in the docs/src/utilities.md
file. You can look at the other sections to see examples of how other docstrings are included.
src/functor.jl
Outdated
""" | ||
f64(m) | ||
|
||
Convert the datatype of model's bias and weights to Float64. |
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.
Convert the datatype of model's bias and weights to Float64. | |
Convert the `eltype` of a model's parameters to `Float64`. |
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.
OK -- I guess your correction is because the datatype of a model is more than that of bias and weights, and may also include parameters in training algorithms, etc.
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.
Something like that. eltype
is a function in Julia that returns the type of each element in an array. So I suggested the change because in Julia lingo, eltype
is a bit more meaningful that "datatype." I also suggested changing "bias and weights" to "parameters" like Dhairya suggested, because not all layers' trainable parameters are referred to as "bias" or "weights" (that's just the case for the common layers like convolutions and fully-connected layers).
Just keep pushing more commits to your branch on git to add new changes. The PR will automatically include them. |
I have updated the docstrings according to proposals. |
Did you mean to close the PR? |
No, not really. I don't know what "close the PR" means. I changed the docstrings according to your proposals, and clicked on Pull request. But this time, I didn't find the button to commit it, or whatever it is called. The procedures are not very clear to me. I know that Git is a version control system, which I think in its raw version operates on my local hard disk. Then, I suspect GitHub and GitLab are cloud services, so that things are stored in the cloud instead on my local hard disk. [But I may be wrong so far...] I suspect that to "fork" the code (or whatever the file or file structure is) means that I make a local clone of the files. I can then edit the forked files. At some stage, I guess I need to tell the Git system that I am happy with my changes. Is this what is called "commit"? Then, think I need to open a Pull request ("PR"), which I suspect means that I make the owner of the original code that I cloned, aware of my proposed changes. In each of these steps, it is not very clear to me what buttons I need to click on, etc. It also further confused me that you guys had inserted comments in my proposals (?? -- something appeared in green and other things in red...) |
No problem, I can understand how this entire process is confusing if it is your first time with Git, let alone Julia and Github! Below is a full summary, but the short of it is that you have done everything correct so far. The suggestions (red/green) are like comments on a Word document but for your code. You can incorporate them by changing the code on your hard drive, committing the changes, and pushing those changes to your fork. Github will automatically update this PR (you do not need to create a new one). We will repeat this process of suggestions + committing changes until the code is ready to be merged into the main Flux repository. You have things mostly correct. Git is a version control system, and it relies on a "remote" repository that people "clone" locally to their hard disks. You make changes locally to the files, and you "commit" those changes when you want to record the changes in the version control system. Commits are a record of changes that will be stored with your local clone of the repository. To send your changes to other people, you "push" your changes to the remote repository. Any remote repository would need to be hosted on a server. Github and Gitlab are cloud services specifically designed to host remote Git repositories. They also have tools for managing the repositories and people's changes to them. This is where "forks" come in. Once you push changes to the remote, it becomes harder to revert those changes especially when many people have cloned copies of the repository. So, the main repository on Github like FluxML/Flux.jl does not allow just anyone to write changes directly to the remote repository. This is why you need to create a fork. When you fork a repository, Github creates a copy of it where you have write access to the remote (e.g. your fork is B-LIE/Flux.jl). Now you can proceed with the process I described in the previous paragraph to make the changes you want to the code and push them to your fork. When you are satisfied with the changes on your fork, that's when a "pull request" or PR comes in. You go to the main repository (e.g. FluxML/Flux.jl) and open a new PR which is a request to pull the changes from your fork into the main remote repository. This is the stage we are at right now. Like I mentioned, Github provides tools to manage how people change the remote repositories. Among those tools are some review tools that allow people to comment on the changes proposed in your PR. These are the inline comments that you saw. The red indicates what the code used to look like before the suggestion, and the green indicates what is being suggested. My comments don't directly change the code on your fork (you can think of them as comments on a Word document). You can accept my changes by clicking the "commit" button under each suggestion, or you can manually make the suggested changes to your local copy on your hard drive. If you do go the manual route, you repeat the process of committing your changes and pushing to your fork. Github will automatically update the PR whenever you push commits to your fork. You can see these updated commits inline with the thread of this PR (they are the little blurbs with your user icon and commit message, "Update functor.jl"). This process repeats iteratively for a PR. We (the maintainers) suggest changes, and you accept or reject those changes until everyone is satisfied with the resulting code. When it is ready, one of the maintainers will submit a review that "approves" the PR, and then we will trigger the process to merge it. Once it is merged (you will see a status change on this page), the process is done. You can delete the code from your hard drive if you like, and you can even delete your forked copy of the repository on Github. Your changes will be part of the main repository of Flux code, and when we make a new release, it will be sent out to users of Flux! |
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.
@DhairyaLGandhi documentation errors look like they are related to the Julia 1.6 update (show changed for vectors and matrices). Don't see anything directly related to this PR. I think we can approve and merge this, and one of us can fix the CI/documentation issue with 1.6 in another PR).
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.
Actually, @B-LIE can you add the docstrings to docs/src/utilities.md
? You can look at other examples in that file for how to do it. The md
file is what is used to generate the HTML documentation for the package.
Thanks for your patience in explaining these things: I really need to learn this. To make things clearer to me, I will try to explain what you wrote, in my own words, which will be some sort of test of whether I really understood it...
You have mentioned a couple of times that I can delete the fork on my local hard drive when I have pushed it.
Anyway, let me know how my understanding is progressing. I'm sorry to bother you about these trivialities, but it is important to me to have a working understanding: it makes it much easier when others ask me to "PR" something... |
Here is the relevant documentation: Should I put the commands in, e.g., "Layer Initialization", or create a new headline? |
The function isn't strictly related to initialization (you can change the parameters to Almost there, but just some small clarifications!
Cloning is the technical term in Git for downloading the remote repository locally to your hard drive. Forking does not involve your local hard drive at all. It is a Github/Gitlab term for creating a new remote repository that is a copy of the repository that you forked. Specifically, https://github.com/FluxML/Flux.jl points to the remote repository on Github owned by FluxML, and your fork, https://github.com/B-LIE/Flux.jl, is a copy of the original, also remote.
The changes will be pushed to your fork (not the main repository), but you are right that the pull request will be update to reflect the changes. Think of a pull request as you proposing changes to the main repository, and your fork is exactly the changes you are proposing. By pushing to your fork, you are implicitly updating the pull request by changing what it is that you are proposing be done to the main repository.
This is something I do not know either 😄. Git is a command line tool. There are many different GUI based software tools that wrap around Git to provide a nicer interface for running Git commands. It sounds like you are using one of these tools, but how and where these tools store the local copy on your hard drive is not part of Git, and it is not standardized. So, maybe if you provide the name of where you downloaded the Git software/tool, I can guess where your files are stored.
The fork lives on Github. The clone of the fork lives on your hard drive. Depending on the tool you are using to do all this, it might integrate with your Github account. In which case, when you use the same tool on your home computer, it will show you the same fork, because it has always existed remotely. Maybe it even downloaded a local clone of the fork to your home computer? I am not sure. |
Perfect -- thanks for your answer! It is getting clearer! Will create a new section in the documentation.I just go to https://github.com/FluxML/Flux.jl in my web browser, sign in to GitHub there, and do the changes in the browser. So it is possible that I only "fork" things in GitHub, and that there is not a true "clone" to my hard disk. To me, that is what would make most sense. But I guess my computer will tell me if I fill up the disk with Git clones... :-) |
Wow, I had no clue you could get all the way from forking to PR within GitHub.com! Today, you taught me something new. Sounds like you're right, you never cloned a local copy of anything onto your hard drive. When you need to do more complex changes to a repository, you might want to work locally instead of within your browser. You'll need to download some software to do that. I like Sublime Merge, but any tool will do. But that's a step in your open source journey for another day probably! |
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.
Just a couple more small changes!
docs/src/utilities.md
Outdated
Flux.f32 | ||
``` | ||
|
||
The default `eltype` for models is `Float32` since models often are trained/run on GPUs. The eltype of model `m` can be changed to `Float64` by `f64(m)`, or to `Float32` by `f32(m)`. |
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.
The default `eltype` for models is `Float32` since models often are trained/run on GPUs. The eltype of model `m` can be changed to `Float64` by `f64(m)`, or to `Float32` by `f32(m)`. | |
The default `eltype` for models is `Float32` since models are often trained/run on GPUs. The `eltype` of model `m` can be changed to `Float64` by `f64(m)`, or to `Float32` by `f32(m)`. |
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.
He, he: I actually first wrote "are often trained", because that sounds better to me. But a lot of people with Latin language background (Italian, Spanish, etc.) think it should be "often are trained" because in Latin based languages, it is "illegal" to split the verb terms = "are trained", i.e., in those languages, it is not acceptable to put a word (such as "often") in between "are trained". I have also met many English speaking who think that verb terms can not be split. Maybe I'm confusing "split infinitive", though.
Anyway, I agree with your proposal, and return to my original phrase ("are often trained"), which also is the required word order in my mother tongue (Norwegian).
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.
Interesting, if I wasn't splitting the verb terms, I would have guessed "are trained often." Language is tricky!
Can you also add the back tick marks around "eltype"?
I tried Gitkraken because a colleague recommended it. He is really negative to anything Microsoft or Apple, so for him, only Linux things matter. But students often use Windows, so he has lowered himself to recommend Gitkraken... |
Thanks @B-LIE for putting in the effort, I'll merge these into the main repo now! bors r+ |
Thanks for your patience and pedagogics for teaching me these things. I hope I don't forget it right away :-o. I guess I need to practice. |
Build succeeded: |
I was challenged to add docstrings to f32 and f64, and have done that.
Since I really never have used Git before, I feel on thin ice here, so I guess the challenge was more to push me out on the ice...