Remove exception due to supportsSparse:true in ReadFromTextLoader

[stephentoub]

In the GitHubLabeler sample, for example, there are ~450 InvalidOperationExceptions getting thrown and caught internally, as the implementation tries to parse some text as an int and fails. There's no need for an exception here; the implementation is already using a TryParse method, which it then turns into an exception and then immediately catches and ignores... we can just cut out the middle man and not throw.

Related to dotnet/machinelearning-samples#257
Related to #2576

[codecov]

Codecov Report

Merging #2579 into master will increase coverage by <.01%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##           master    #2579      +/-   ##
==========================================
+ Coverage    71.5%   71.51%   +<.01%     
==========================================
  Files         801      801              
  Lines      142023   142024       +1     
  Branches    16147    16148       +1     
==========================================
+ Hits       101557   101563       +6     
+ Misses      35998    35993       -5     
  Partials     4468     4468

Flag	Coverage Δ
#Debug	71.51% <70%> (ø)	⬆️
#production	67.79% <70%> (ø)	⬆️
#test	85.57% <ø> (ø)	⬆️

[codemzs]

GetValueOrDefault [](start = 32, length = 17)

Why not just GetValue() since you have already checked it has value? Alternatively we can remove this if condition and do the below:

value = (I4)res.GetValueOrDefault();
return res.HasValue;

[stephentoub]

Why not just GetValue() since you have already checked it has value?

Exactly because we already checked it has a value. GetValueOrDefault() just does return value; whereas .Value does something like if (!_hasValue) throw new Exception(); return _value;.

[codemzs]

Thanks for the explanation. My understanding of these routines was the exact opposite of what they really do. Looks good!

[TomFinley]

Hmmm. We may have some code to rewrite a number of places... opened #2612 to track this.

[codemzs]

[TomFinley]

Thank you @stephentoub .

[eerhardt]

Why not use the existing TryParse method?

machinelearning/src/Microsoft.ML.Data/Data/Conversion.cs

Lines 1184 to 1201 in 369a9b6

	/// <summary>
	/// This produces zero for empty. It returns false if the text is not parsable or overflows.
	/// On failure, it sets dst to the defualt value.
	/// </summary>
	public bool TryParse(in TX src, out I4 dst)
	{
	dst = default;
	TryParseSigned(I4.MaxValue, in src, out long? res);
	if (res == null)
	{
	dst = default;
	return false;
	}
	Contracts.Assert(res.HasValue);
	Contracts.Check((I4)res == res, "Overflow or underflow occured while converting value in text to int.");
	dst = (I4)res;
	return true;
	}

[stephentoub]

I was trying to remain consistent with the existing code, which assigned a value, then passed it by ref, and then checked whether it remained what it was assigned. In practice it wouldn't matter because it would end up with the default value in either case, but it would incur additional writes in the failure case, and I didn't know whether it would matter. When I don't know a code base particularly well and what ramifications my changes might have, I try to maintain the existing code as much as possible.

[eerhardt]

I guess my concern is the consistency in the internal Conversion class.

1. We now have TryParse and TryConvert. Which one should new callers choose?
2. We have TryConvert for Int32, but no other types?

[stephentoub]

1. Whichever signature they need. If they want an out, they choose TryParse, if they want ref, they choose TryConvert.
2. Yes. You'd prefer we add a bunch of dead code?

If you feel strongly about this and want to change the call site to use TryParse, go for it. As I noted, my goal was to muck minimally with the call site logic.