Logical bug in IRTNet.forward

[niall-twomey]

🐛 Description

I was browsing this repo for IRT implementations and found (I think) a theoretical bug in the implementation of IRTNet.

IRTNet.forward is defined here
https://github.com/bigdata-ustc/EduCDM/blob/main/EduCDM/IRT/GD/IRT.py#L30

    def forward(self, user, item):
        theta = torch.squeeze(self.theta(user), dim=-1)
        a = torch.squeeze(self.a(item), dim=-1)
        b = torch.squeeze(self.b(item), dim=-1)
        c = torch.squeeze(self.c(item), dim=-1)
        return torch.sigmoid(self.irf(theta, a, b, c, **self.irf_kwargs))

And the logic is that the output of irf is passed through the sigmoid function. This is fine if the output of irf itself is a "logit".

The IRF function is defined here:
https://github.com/bigdata-ustc/EduCDM/blob/main/EduCDM/IRT/irt.py#L10

def irf(theta, a, b, c, D=1.702, *, F=np):
    return c + (1 - c) / (1 + F.exp(-D * a * (theta - b)))

If you look at this you can see that it is already depicting sigmoid behaviour (assuming, of course, that 0 <= c <= 1). In other words, irf is returning probabilities, and not logits. As a result, the forward function above is actually doing this:

1 / (1 + exp(-(c + (1 - c) / (1 + F.exp(-D * a * (theta - b)))))

which I think is probably a bug.

If I haven't misunderstood, I have two recommendations:

• Simply remove the torch.sigmoid call from forward
• (optional) it may be worth passing c through a sigmoid function to ensure it doesn't go negative or above 1. (Perhaps selectable in irf_kwargs?)

i.e.

    def forward(self, user, item):
        theta = torch.squeeze(self.theta(user), dim=-1)
        a = torch.squeeze(self.a(item), dim=-1)
        b = torch.squeeze(self.b(item), dim=-1)
        c = torch.squeeze(self.c(item), dim=-1)
        if self.irf_kwargs.get("squash_c", True):
            c = torch.sigmoid(c)
        return self.irf(theta, a, b, c, **self.irf_kwargs)  # May want to clip values if c not constrained

Edit: I noticed that this torch.sigmoid(irf(...)) pattern also happens in MIRT, and possible elsewhere too.
Edit 2: I also realise that because sigmoid is monotonic, it doesn't really change the optimal solution. However, it does seem unnecessary to differentiate through sigmoid twice.

---

Error Message

NA

To Reproduce

NA

Environment

Environment Information

Operating System: NA

Python Version: NA

Additional context

[tswsxk]

Thank you for your bug report, we will reply you as soon as possible.

[ViviHong200709]

Thanks for reporting the bug and you provided very useful advice on the improvement of our framework .We fix the bug by removing the sigmoid function. We also add constrains that ensures a>=0 and 0 <= c <= 1.

[niall-twomey]

@ViviHong200709 - thanks! I added two comments to the PR that I think should be looked at.