search

civisanalytics / civis-python

Civis API Python Client
BSD 3-Clause "New" or "Revised" License
34 stars 26 forks source link

BUG fix civisml 1.0 prediction template id #280

Closed elsander closed 5 years ago

elsander commented 5 years ago

I noticed that both CivisML 1.0 and 1.1 had the same prediction id. This PR corrects the 1.0 template id.

elsander commented 5 years ago

I did some git blame archaeology here, and it looks like this was an intentional change to ensure that the most up-to-date prediction template within the major version. This isn't the process we've been following in 2.0; we've been keeping training and prediction matches at the minor version.

I personally lean toward keeping templates matched up at the minor version level, for the sake of reproducibility. Either way, I'd rather be consistent in how we're doing this between versions 1 and 2. What are your thoughts @stephen-hoover?

stephen-hoover commented 5 years ago

Yes, we'd originally planned to always make predictions with the most recent minor version, but discovered that wouldn't always work.

I agree, for all major versions we should use the prediction job minor version corresponding to the training job. Hopefully no one is still using v1, but the consistency would be a good idea anyway.