Add a version attribute to the DetectorDatabase class

[BSchelpe]

This should close #575

Two comments about the code:

1. I'm not sure what numbering system to use for the version. It looks more like a version number in the format a.b, although it seems a bit overkill to distinguish changes warranting an increment in a vs an increment in b. On the other hand using a single number, a, looks odd. My slight preference is for a.b, leaving it up to the coder to choose whether their change is major or minor. There is also a question of what number to start from. I set it to 1.0, but possibly 0.1 would be better, more in keeping with the TQEC version being 0.01 at the moment?
2. One suboptimality in the code is that ideally I would like database instances saved before this code change (ie with no version number) to be treated as having the initial version number (1.0 at the moment). However, due to the way pickle loading works they are loaded with the current version number of the code. This is only problematic if someone tries to load an old database instance (pre code change) after a long time when the database version has increased from 1.0 (which we don't anticipate happening very often). I hope it is enough to warn the user about this in the documentation, as I don't see a neat way to avoid it.

[nelimee]

In my opinion, the most pythonic way of handling versions is to use semver. From the package documentation:

The module follows the MAJOR.MINOR.PATCH style:

• MAJOR version when you make incompatible API changes,
• MINOR version when you add functionality in a backwards compatible manner, and
• PATCH version when you make backwards compatible bug fixes.

For this specific case, and in my opinion:

• MAJOR changes when the format of the file changes (i.e., when the attributes of DetectorDatabase change),
• MINOR changes when the content of the database is invalidated (e.g., by changing a plaquette implementation without changing its name),
• PATCH does not need to change.

[BSchelpe]

In my opinion, the most pythonic way of handling versions is to use semver. From the package documentation:

The module follows the MAJOR.MINOR.PATCH style:

• MAJOR version when you make incompatible API changes,
• MINOR version when you add functionality in a backwards compatible manner, and
• PATCH version when you make backwards compatible bug fixes.

For this specific case, and in my opinion:

• MAJOR changes when the format of the file changes (i.e., when the attributes of DetectorDatabase change),
• MINOR changes when the content of the database is invalidated (e.g., by changing a plaquette implementation without changing its name),
• PATCH does not need to change.

That makes sense to me. I'll add your guidance on what constitutes a major/minor change to the doc string. Given that does starting the version at 0.1 make sense?

[nelimee]

That makes sense to me. I'll add your guidance on what constitutes a major/minor change to the doc string. Given that does starting the version at 0.1 make sense?

I may be wrong, but for me the 0 major version in semantic versioning is slightly special. It basically means "this is still an alpha/beta, so each minor version change may be incompatible". As such, and because the DetectorDatabase currently works, I would start by 1.0.

[BSchelpe]

That makes sense to me. I'll add your guidance on what constitutes a major/minor change to the doc string. Given that does starting the version at 0.1 make sense?

I may be wrong, but for me the 0 major version in semantic versioning is slightly special. It basically means "this is still an alpha/beta, so each minor version change may be incompatible". As such, and because the DetectorDatabase currently works, I would start by 1.0.

Ah that makes sense I was a bit unclear on the 0/1 difference.

[BSchelpe]

@nelimee this is ready for final review, when you have time :)

[nelimee]

@BSchelpe looking again at the code, I think the None trick for the database makes everything more complex. Removing the "developers should set XXXX to None when they want to regenerate the database each time" requirement for this PR makes it way simpler, and that requirement can be addressed differently.

What is your opinion on the above?

[BSchelpe]

@BSchelpe looking again at the code, I think the None trick for the database makes everything more complex. Removing the "developers should set XXXX to None when they want to regenerate the database each time" requirement for this PR makes it way simpler, and that requirement can be addressed differently.

What is your opinion on the above?

That's a good point. I think I agree. In the short term, the do_not_use_database flag can be used for developers wanting the behaviour, so the functionality is there, if not as convenient as would be ideal. I'll strip out the None option from the PR.

[nelimee]

Disregard my previous message, I reviewed the wrong commit and did not pull your changes. Doing the review again :)

[BSchelpe]

@nelimee I think this is all done now - your suggestions are incorporated and I have removed the None option. I think this is mergeable once you're happy with it. Thank you!