Issue #567: define loss as classes and use registry#568
Conversation
Codecov Report
@@ Coverage Diff @@
## main #568 +/- ##
==========================================
+ Coverage 99.44% 99.48% +0.03%
==========================================
Files 34 36 +2
Lines 1997 2131 +134
==========================================
+ Hits 1986 2120 +134
Misses 11 11
Continue to review full report at Codecov.
|
mathpluscode
changed the title
|
@YipengHu @acasamitjana @zacbaum @s-sd Would you have time review this PR ^^ In general, I
|
|
Please check if docs that need to update... |
I don't think so, as there were no docs and @NMontanaBrown was writing it ^^ in #559. |
|
This looks good! I will send my comments/reviews in the next few days. The main concern is that the APIs are technically unchanged, while the config files are no longer back-compatible. What is the best way to inform users - i would even consider a simple script to convert from the old format to new one? any idea? @zacbaum @NMontanaBrown |
do we have users....? |
…asses-instead-of-functions
|
@zacbaum if you have time for this huge PR :) but no rush ^^ |
|
Thanks for doing this @mathpluscode - It was clearly lots of work! All looks good as is; though I agree with @YipengHu:
Is there no (non-trivial) to support both? I know that this likely means lots of duplicate code... But maybe better than just ridding the codebase of the previous way that things were done. Otherwise, this is a borderline major change as far as semver goes.
I'd agree with this; or we deprecate it but leave it in the codebase for the time being and log a warning to the user and provide a link/info about how to update their config file. Then remove it in a later version. Allow any possible users the chance to update before we just remove it entirely. |
|
ok i solved the comments, @NMontanaBrown, for the docs: as mentioned (maybe multiple times) I will do a doc in the end when I'm 100% sure about the registry, I still have multiple PRs to do, and this is also a reason why we have |
|
You mention registry is not done, what is missing for registry? If we are not sure about it as a feature 100%, we should not be merging it into the main branch and it should be maintained as a WIP PR. Irregardless I still think it is a very big change from our current working pipeline so explanations about it should be added in some form of documentation WITH the merge, whenever it happens, not separately, as this is a non-intuitive, non-minor change. |
The registry for now is not for users, and users do not need to know anything with registry, they only need to provide correct configs. So i dont know what kind of docs you would like to have. The docs we need will be in your branch. When I say ready, i mean user will be able to use the registry with their custom classes. For this we have other issues open already. I need to merge this as it is now blocking many things ^^ and tbh it's not a dirty merge. |
|
@NMontanaBrown if you can clarify a bit more on the docs you would like to have? |
First, the changelog has been updated. From the PR, what I can see needs documentation support is mainly due to the format change of the config files. This was why I brought up the potential conflict with the Nina's ticket. While there is no reason doc and code need to be in a single PR, if the relevant doc is non-existent or insufficient - there are plenty of undocumented features in every software. One solution would be merge #525 first to this PR (this might be perceived to reduce the chance of further tech debt, but also has a price - more on this later). If #525 can be done quickly, then this is definitely an option nonetheless. @mathpluscode what is specifically being blocked by this PR and cannot merge this branch to first instead? For compatibility issue for the config change, several of us agreed that the added error messages is the best to mitigate the risk of confusing users. We discussed this in length, trying to balance between a very wordy tedious explanation to "old users" and simplicity-and-clarity. Personally, I believe it is adequate for now. And the fact that the changed config is more structured, simplified and clearer now, with #525, we should have a better overall presented config. @NMontanaBrown Do you have any other specific suggestion to further improve it? Even the answers to both above questions are nos, I suggest to merge this asap, simplify due to "the principle of being agile and snappy" ;) As we discussed many times, another type of tech debt will build up quickly too, for example every newly created branch will need to rebase this big PR before merge. |
Ideally we need proper API docs and a page in the docs for registry - this will take more time and is the complete solution and we need to do it so we should raise a ticket before package release or do it now. Ideally, do it now, because that way we provide a complete, and not half baked code solution into the main branch without documentation on how to use new functionality. @mathpluscode EDIT: I see this is a ticket at #585. Then a release with a small .md on registry if fine For this PR, there should at least be a .md file that at least explains nominally WHY we have changed from classes -> registry workflow, and also provide some pointers on how this works so nominally users will be able to debug their own code if they are coding from main branch and using config files that will no longer be supported. |
…asses-instead-of-functions
I've added the doc ^^ have a look https://deepreg--568.org.readthedocs.build/en/568/docs/registry.html |
…asses-instead-of-functions
…asses-instead-of-functions
…asses-instead-of-functions
| dictionary mapping `(category, key)` to `value`. It also provides the | ||
| `build_from_config` functionality, which allows creating a class instance from a config | ||
| directly. This allows the simplification of the config so that each configuration file | ||
| need to provide the name and necessary arguments. |
There was a problem hiding this comment.
Can change line 18 to "This allows for the simplification of the config file such that each configuration only needs to provide the name of the class and the arguments to build the class for use. Documentation on Registry config will be released with issue #525 ."
Bit more comprehensive?
There was a problem hiding this comment.
i added, except the last bit about doc, as it's mentioned in the title.
| > This is a functionality under active development. A full documentation will be | ||
| > released later. | ||
|
|
||
| DeepReg is adapting the usage of the registry system recently, to facilitate the usage |
| need to provide the name and necessary arguments. | ||
|
|
||
| With the registry, when developing new classes **inside DeepReg**, we should use the | ||
| corresponding decorator, so that the class is registered, cf. |
There was a problem hiding this comment.
I don't understand this either
There was a problem hiding this comment.
|
@mathpluscode @NMontanaBrown I see your registry page now - only minor typos, otherwise fine to me. |
NMontanaBrown
left a comment
NMontanaBrown
left a comment
There was a problem hiding this comment.
Looks good, thanks @mathpluscode , agree with Yipeng, typo fix + 1 modif and approve.
|
@YipengHu @NMontanaBrown done ;) have another look plz |
mathpluscode
deleted the
567-define-loss-as-classes-instead-of-functions
branch
4 participants
Description
Fixes #567
Type of change
What types of changes does your code introduce to DeepReg?
Please check the boxes that apply after submitting the pull request.
Checklist
Please check the boxes that apply after submitting the pull request.
If you're unsure about any of them, don't hesitate to ask. We're here to help! This is
simply a reminder of what we are going to look for before merging your code.
installed pre-commit
using
pre-commit installand formatted all changed files. If you are notcertain, run
pre-commit run --all-files.our requested structure,
e.g.
Issue #<issue number>: detailed message.change log file
regarding my changes.