Replicated softmax#137
Conversation
|
Hi Federico, regarding the Python 3.8 error, it seems that some libraries have dropped support for python 3.8. I think it's time to deprecate it. Could you please remove Python 3.8 from python-publish.yml and any other related references? Also, please add tests to ensure everything works as expected. I don't currently have time to verify the implementation manually, but adding tests will give us more confidence and serve as guardrails. You can follow the same patterns as in: https://github.com/MIND-Lab/OCTIS/blob/master/tests/test_octis.py and https://github.com/MIND-Lab/OCTIS/blob/master/tests/test_optimization.py. Lastly, could you run a formatter (e.g., ruff) on the files you added? Thanks, Silvia |
silviatti
left a comment
silviatti
left a comment
There was a problem hiding this comment.
Please review the comments in the code. I recommend running a linter to check for unused functions, duplicated code blocks, and other potential issues. I also asked ChatGPT for a quick review and it identified several points worth examining.
I am not an expert in RSM, so I can only validate those findings to a certain extent. I suggest doing a similar review yourself to further assess and verify potential issues in the implementation.
| def _get_topic_word_matrix(self): | ||
| """ | ||
| Return the topic representation of the words | ||
| """ | ||
| w_vh, w_v, w_h = self.W | ||
| topic_word_matrix = w_vh.T | ||
| normalized = [] | ||
| for words_w in topic_word_matrix: | ||
| minimum = min(words_w) | ||
| words = words_w - minimum | ||
| normalized.append([float(i) / sum(words) for i in words]) | ||
| topic_word_matrix = np.array(normalized) | ||
| return topic_word_matrix | ||
|
|
||
| def _get_topic_word_matrix0(self): | ||
| """ | ||
| Return the topic representation of the words |
There was a problem hiding this comment.
these functions are duplicated
| visible_sample = self.sample_softmax(visible_probs, D) | ||
| return visible_sample | ||
|
|
||
| def sample_h2(self, h1): |
There was a problem hiding this comment.
is this function used? if not, remove
| mu1 = self.v_and_h2_to_h1(v, h2) | ||
| mu2 = self.h1_to_softmax(mu1) | ||
|
|
||
| if (old_mu2 - mu2).sum() < self.epsilon: |
There was a problem hiding this comment.
are you sure about this? I asked Chatgpt and they say it's incorrect because:
- Differences may cancel out (positive/negative)
- It does not use absolute values
It should be:
if np.linalg.norm(old_mu2 - mu2) < self.epsilon:or
if np.sum(np.abs(old_mu2 - mu2)) < self.epsilon:There was a problem hiding this comment.
I agree, that's a big point. That error is also present in the original version proposed by dongwookim. Maybe I will have to tune the default value of epsilon to make it larger and let a faster convergence.
There was a problem hiding this comment.
I have done the tests and the convergence is reached with a reasonably small epsilon, so I have applied the changes.
| else: | ||
| self.dtm = dtm | ||
| if doval: | ||
| self.val_dtm = np.log(1 + val_dtm) |
There was a problem hiding this comment.
is it correct that you log transform the validation?
There was a problem hiding this comment.
No, it is really wrong, I will correct it. The log transform should be applied only when the user requires it, for both training and validation (it should be considered a preprocessing step).
| else: | ||
| self.dtm = dtm | ||
| if doval: | ||
| self.val_dtm = np.log(1 + val_dtm) |
There was a problem hiding this comment.
see similar comment in oRSM
| """ | ||
| mfh = self.visible2hidden(dtm) | ||
| vprob = self.hidden2visible(mfh) | ||
| lpub = np.exp(-np.nansum(np.log(vprob) * dtm) / np.sum(dtm)) |
There was a problem hiding this comment.
is this log perplexity or simply perplexity?
There was a problem hiding this comment.
Simply perplexity, I have to correct it or remove the np.exp
| given a document term matrix | ||
| """ | ||
| mfh = self.visible2hidden(dtm) | ||
| vprob = self.hidden2visible(mfh) |
There was a problem hiding this comment.
possibily clip vprob (e.g. vprob = np.clip(vprob, 1e-12, None)) to avoid vprob=0 and Nan values later?
There was a problem hiding this comment.
also, can np.sum(dtm) = 0?
There was a problem hiding this comment.
For the first problem, your clip solution is more elegant and efficient than my sad np.nansum operation, so I will use it.
For the second, I give an answer similar to the D=0 problem above.
It shouldn't be possible that dtm is a matrix of zeros. The dtm columns are as many as the words in the vocabulary initialized by the preprocessed corpus. To handle a case were the vocabulary is given and the corpus contains empty documents, an assert check can be added to solve it.
Maybe an error can be thrown in this way?
assert np.sum(dtm)==0, 'All the documents in the corpus seems to be empty for the given vocabulary'
| DTM[i, id] = count | ||
| return DTM | ||
|
|
||
| class oRSM_model(object): |
There was a problem hiding this comment.
I would recommend implementing a shared superclass which implements the shared functions, while you override the model-specific functions. This prevents you from having a lot of duplicated code e.g. _get_topic_word_matrix, get_topics, softmax, multinomial_sample, set_train_hyper, etc.
There was a problem hiding this comment.
Ok, I will add a third file, RS_utils.py, together with RSM.py and oRSM.py in the models folder.
There was a problem hiding this comment.
I have called the file RS_class.py, where the shared superclass is implemented. I hope I've done it in the way You mean. Many functions (like set_train_hyper) are quite different between the two classes, despite the common names they have.
| 'Programming Language :: Python :: 3', | ||
| 'Programming Language :: Python :: 3.7', | ||
| 'Programming Language :: Python :: 3.8', | ||
| #'Programming Language :: Python :: 3.7', |
|
Hi Silvia, thanks for the review, it surfaces important problems. |
…ect RS and oRS to handle errors of empty documents.
|
Hi Silvia, I have inserted the duplicated functions in RS_class.py , and removed them from the other scripts. Let me know If I have done everything in the good way, or if I have to correct something else. Thank You for your time. Federico |
| converge = False | ||
| mu2 = np.random.random(self.visible) * self.M # initialize mu2 randomly | ||
|
|
||
| while not converge: |
There was a problem hiding this comment.
wondering if it'd be better to have a max number of iterations instead of an infinite loop and warn if convergence is not reached
There was a problem hiding this comment.
Ok I have added a new hyperparameter: max_iter_mfa. Default is 20.
I have also added a warning to advise for each document if it's latent variables are not converging.
However, convergence seems to be reached for any document inside the 20 NewsGroups dataset in less than 5 iterations.
So luckily this problem is not so present.
| adam_m2_vh = np.zeros((dictsize, num_topics)) | ||
| adam_m2_v = np.zeros((dictsize)) | ||
| adam_m2_h = np.zeros((num_topics)) | ||
| t = 1 |
There was a problem hiding this comment.
shouldn't this start at t=0?
There was a problem hiding this comment.
The reason is mathematical: t appears in the bias correction denominators, inside self.gradient_adam, and starting at 0 would cause a division by zero.
bias_correction1 = 1 - decay1t
bias_correction2 = 1 - decay2t
If t = 0 on the first iteration (before incrementing) they both become 1-1=0 and then
adam_m1_vh_hat = adam_m1_vh / 0 # NaN / inf
adam_m2_vh_hat = adam_m2_vh / 0 # NaN / inf
There was a problem hiding this comment.
bias_correction1 = 1 - decay ^ t
bias_correction2 = 1 - decay ^ t
There was a problem hiding this comment.
No, sorry, I have reviewed the function and You are right, the starting value should be 0 since the increment happens before the bias_correction assignments.
| rms_m2_vh = np.zeros((dictsize, num_topics)) | ||
| rms_m2_v = np.zeros((dictsize)) | ||
| rms_m2_h = np.zeros((num_topics)) | ||
| self.rms_decay = 0.9 |
There was a problem hiding this comment.
does this overwrite the user-defined parameters?
There was a problem hiding this comment.
Yes, sorry. I added them last to the main hyperparameters.
| self.adam_decay1 = 0.9 | ||
| self.adam_decay2 = 0.999 |
There was a problem hiding this comment.
does this overwrite the user-defined parameters?
There was a problem hiding this comment.
Yes, sorry. I added them last to the main hyperparameters.
There was a problem hiding this comment.
I have applied the same changes to RSM.py
| self.mean_h = True # whether to use mean hidden activations or sample them | ||
|
|
||
| self.btsz = btsz | ||
| self.batches = int(np.floor(N / btsz)) |
There was a problem hiding this comment.
last batch seems to be discarded. maybe
self.batches = int(np.ceil(N / btsz)) and then ids = np.arange(start_id, min(start_id + self.btsz, N))
There was a problem hiding this comment.
Ok, I didn't notice that. I have changed the code lines so that now works with this logic.
| self.val_dtm = val_dtm | ||
|
|
||
| D = self.dtm.sum(axis=1) | ||
| assert not np.any(D == 0), "all the documents should have positive length" |
There was a problem hiding this comment.
it might be better to raise an error here
There was a problem hiding this comment.
should we validate val_dtm too if it's not none?
| def neg_free_energy(self, v): | ||
| """ | ||
| given an array v similar to the dtm, computes the | ||
| log pdf under the replicated softmax | ||
| """ | ||
| w_vh, w_v, w_h = self.W | ||
| T = self.hidden | ||
| D = v.sum(axis=1) | ||
| fren = np.dot(v, w_v) | ||
| for j in range(T): | ||
| w_j = w_vh[:, j] | ||
| a_j = w_h[j] | ||
| fren += np.log(1 + np.exp(D * a_j + np.dot(v, w_j))) | ||
| return fren | ||
|
|
||
| def neg_free_energy_single_doc(self, v): | ||
| """ | ||
| given a one dimensional Bow vector v representing a single document, |
There was a problem hiding this comment.
these functions look very similar. maybe consolidate a single helper?
There was a problem hiding this comment.
Ok, the unique difference between the two functions was just the shape of D (the total words in the document).
Now the function neg_free_energy works as both.
| monitor_loglik=False, | ||
| logdtm=False, | ||
| ): | ||
| """function to initialize the weigths matrices |
There was a problem hiding this comment.
weigths is mispelled several times
There was a problem hiding this comment.
Sorry for my english.
| function to adjust the gradient of the | ||
| topic-word interaction weights during a training iteration | ||
| of a RS model by a penalty factor. | ||
| The model shoud have the attributes: |
There was a problem hiding this comment.
typo corrected, thanks
| @@ -0,0 +1,263 @@ | |||
| import numpy as np | |||
There was a problem hiding this comment.
it seems that many of these functions can overflow. Not sure if how realistic this is in practice, but it might be worth validating this
There was a problem hiding this comment.
I've made more numerically stable the functions sigmoid, softmax_vec and neg_free_energy.
I wouldn't add error handling inside these functions.
PR Summary from CopilotStatus:
Core ChangesThis PR adds two new topic modeling algorithms to OCTIS:
Both models support classification, diversity, and coherence evaluation metrics, plus perplexity computation via approximations and Annealed Importance Sampling (AIS). Key Implementation Files# Base class providing shared functionality:
# - Activation functions (softmax, sigmoid)
# - Sampling methods (multinomial, Bernoulli rejection)
# - Likelihood computation (free energy)
# - Topic-word matrix output
# - Weight initialization# RSM model implementation with support for:
# - Multiple CD variants (mean-field, k-step, persistent, gradual)
# - Multiple optimizers (SGD, momentum, RMSProp, Adam, Adagrad)
# - Perplexity approximation and exact AIS estimation# oRSM model with deeper architecture:
# - Pretraining phase + main training phase
# - Mean-field approximation for hidden layers
# - Similar optimizer and CD options as RSMOther Changes
Merge Readiness & Risk Assessment🔴 Critical Issues (Must Address Before Merge)1. Code Quality & Duplication
2. Convergence Loop Issues
3. Numerical Stability Issues
4. Batch Processing Bug
|
| Author | Type | Status |
|---|---|---|
| silviatti | Changes Requested | Active (most recent 22 May 2026) |
| Fede-Rausa | Commented | Acknowledges most issues |
Key feedback (31 inline comments):
- Code organization and duplication concerns
- Numerical stability edge cases
- Potential infinite loops
- Documentation/naming clarity
Possible Improvements
- Refactor shared code into utility functions or extend base class further
- Add max iteration limits to all convergence loops with warnings
- Standardize probability handling with consistent clipping logic
- Add comprehensive docstrings explaining mathematical models
- Consider caching for expensive computations (e.g., softmax during sampling)
- Add logging/debugging output option for troubleshooting convergence
Note: these should be taken as suggestions rather than strict requirements.
|
Hi Silvia! To make the build possible, I had to change the requirements.txt, replacing spacy with spacy==3.7.6 (that is the last version of spacy that was compatible with python 3.9). Let me know If I have done everything right, or have to correct something else. Thank You for your time. I'm at disposal. Federico |
2 participants
Adding Replicated Softmax (a restricted boltzmann machine for topic modeling) and Over Replicated Softmax (a deep version of the RSM) models to the octis topic models list.
Both can be currently evaluated only on classification, diversity and coherence metrics, but support locally a fast computation for the perplexity upper bound (that is supported also for the LDA in the gensim implementation, used by octis).