Skip to content

Fixes #4385 about calling the Create methods when loading models from disk #4485

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Nov 27, 2019
Merged

Fixes #4385 about calling the Create methods when loading models from disk #4485

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d8e1dfc
Loading both create and constructor method and log when both of them …
antoniovs1029 Nov 6, 2019
e7f0910
Also log parmTypes
antoniovs1029 Nov 8, 2019
1e5170a
Changed access modifiers of some classes
antoniovs1029 Nov 18, 2019
b1e0761
Added filtering between nonpublic methods
antoniovs1029 Nov 18, 2019
a1a430d
Changed visibility of several classes
antoniovs1029 Nov 18, 2019
dd62e51
Adding more logs
antoniovs1029 Nov 18, 2019
e076101
Change access modifier of public constructors
antoniovs1029 Nov 18, 2019
9841ed5
Merge remote-tracking branch 'upstream/master' into is07createDexper
antoniovs1029 Nov 18, 2019
87c1b5a
Change access modifier of SchemaBindableCalibratedModelParameters class
antoniovs1029 Nov 18, 2019
33fb93c
Updated comments and removed logging methods
antoniovs1029 Nov 18, 2019
bb408f4
Change if statements for 'AccessModifier' extension method
antoniovs1029 Nov 19, 2019
26609e8
Added PrivateProtected to enum and other minor changes
antoniovs1029 Nov 19, 2019
ad833ad
Change exception message
antoniovs1029 Nov 26, 2019
88aef21
Moving assignement of create and ctor inside if condition
antoniovs1029 Nov 26, 2019
1dd3e33
Erased redundant 'else' statements since 'if' statements where alread…
antoniovs1029 Nov 26, 2019
d5a0b9a
Merge remote-tracking branch 'upstream/master' into is07createDexper
antoniovs1029 Nov 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Change exception message
  • Loading branch information
@antoniovs1029
antoniovs1029 committed Nov 26, 2019
commit ad833adc31e9a9ea91eada86ac872e5e2c3ecd31
2 changes: 1 addition & 1 deletion src/Microsoft.ML.Core/ComponentModel/ComponentCatalog.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ private static bool TryGetIniters(Type instType, Type loaderType, Type[] parmTyp

if (ctor.Accessmodifier() == create.Accessmodifier())
{
Copy link
Member Author

@antoniovs1029 antoniovs1029 Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So I changed the nested if's I had (link to comment) for this other solution using the Accessmodifier() extension method (as suggested by @yaeldekel ). Although I think this one is more legible, I wouldn't be sure if it's worth it to create the extension method only for this.... So let me know your opinions, Thanks! #Resolved

Copy link

Choose a reason for hiding this comment

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

To me this seems cleaner.

A couple of more ways you can decrease the amount of "if/else"s in the code:

  1. you can put the assignment inside the if condition, like this:
if ((ctor = loaderType.GetConstructor(...)) == null)
  1. If you throw or return inside the "if", then you don't need the "else":
if (ctor.Accessmodifier() == create.Accessmodifier())
    throw ...
if (ctor.Accessmodifier() > create.Accessmodifier())
{
    ...
    return true
}
if (ctor.Accessmodifier() < create.Accessmodifier())
...
etc.

In reply to: 347688260 [](ancestors = 347688260)

throw Contracts.Except($"Can't load type {instType}, because it has both create and constructor methods with the same visibility. Please open an issue for this to be fixed.");
throw Contracts.Except($"Can't load type {instType}, because it has both create and constructor methods with the same visibility. Please indicate which one should be used by changing either the signature or the visibility of one of them.");
}
else if (ctor.Accessmodifier() > create.Accessmodifier())
{
Expand Down