Rename classifier (for NRE) to critic

[bkmi]

What does this implement/fix? Explain your changes

The name classifier is inaccurate. Rather, a general name such as critic is more appropriate. The critic can be transformed into a classifier using the sigmoid (or other transformation). It can be made to estimate the likelihood-to-evidence ratio by simple evaluation. However, I think keeping the name of the network distinct from the RatioEstimator class is a good idea.

Does this close any currently open issues?

#1079

Any relevant code examples, logs, error output, etc?

Nope

Any other comments?

This cannot be applied until #1097 has been pulled!

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. 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.

• I have read and understood the contribution
  guidelines
• I agree with re-licensing my contribution from AGPLv3 to Apache-2.0.
• I have commented my code, particularly in hard-to-understand areas
• I have added tests that prove my fix is effective or that my feature works
• I have reported how long the new tests run and potentially marked them
  with pytest.mark.slow.
• New and existing unit tests pass locally with my changes
• I performed linting and formatting as described in the contribution
  guidelines
• I rebased on main (or there are no conflicts with main)

[janfb]

Great! thanks a lot Ben 🙏
looks all good. I am just not sure about the renaming to "critic". Technically, I see your point. But I fear it will be less clear for users. It also breaks the API. But I am open to be convinced.

[janfb]

what is the reason for this change? Was this a bug?

[bkmi]

.numel() returns the number t.flatten().shape()[0], but this gives only the last dimension. I like thinking about dimensions when I can. Personally I think this makes more sense since we force the user to give (Batched) 1d inputs to these networks.

[bkmi]

I will follow up after Friday.

[bkmi]

@janfb, I used critic since it was shorter than discriminator. Also critic is the name used in WGAN where the "discriminator" network does not act as a binary classifier. We are in that situation, so I chose the name critic. However, I don't have a strong opinion except not to call it a classifier. There's a slight bias against discriminator for me since discriminators in GANs are classifiers.

let me know how you like it and I'll change to to that so we can get this merged.

[janfb]

thanks for the update @bkmi !

I see your point of classifier vs discriminator vs critic. However, changing the name would result in a major API change. We would need to make sure it's back compatible, i.e., catch all classifier in arguments and (user facing) function names etc. and issue a deprecation warning.
I would be hesitant in doing all this unless we really need to :)
@michaeldeistler what do you think?

[michaeldeistler]

I would also prefer to not change this right now -- users will already get a bunch of warnings when the new version is released (thinning for MCMC, importing posterior_nn) so I would prefer to not change this.

[bkmi]

okay, I'll follow your lead, but I would argue that maybe having one big change with many warnings will be simpler than adapting code once, then again for a later additional change.

[janfb]

Hi @bkmi to move on with this PR I suggest the following:

As we would prefer to not do the renaming to critic, we should drop those changes. The PR also contains a couple of other changes that we do want to keep though. However, it seems that those changes are contained in your other PR as (see #1097 ). Thus, could you please close this PR and double check that all not renaming related changes end up in #1097 ?

Thanks a lot! 🙏