Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update batch_viewer docs to accurately reflect data indexing #176

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeffreygwang
Copy link

A change to address the issue below.

Issue: Documentation for Step Viewing

Currently, the documentation for utils/batch_viewer.py reads as follows:

parser.add_argument(
        "--start_iteration",
        type=int,
        default=0,
        help="What train step to start logging"
    )
    parser.add_argument(
        "--end_iteration",
        type=int,
        default=143000,
        help="Train step to end logging (inclusive)"
    )

In normal Python ranges (e.g. range(a,b), the first number is inclusive, so this would imply to me that a start of 1 and an end of 5 should include data from steps 1, 2, 3, 4, and 5.

However, a small experiment reveals this is not the case, as one cannot get a full batch of data by giving a start/end that are one apart (you would expect there to be two batches of data here in an inclusive/inclusive world):

python utils/batch_viewer.py --start_iteration 0 --end_iteration 1 --load_path final/document --save_path tester --conf_dir utils/dummy_config.yml
... [output] ...
(base) bash-4.4$ cd tester
(base) bash-4.4$ python
>>> import numpy as np
>>> zeroone = np.load("indicies.npy")
>>> zeroone.shape
(1025, 2049)

Similarly, when using the same number for start and end, you get a np array of shape (1, 2049).

This implies either the beginning is exclusive, or the ending is not inclusive as documented. I put in a PR with the former, but please let me know / change it if it's the latter!

Change documentation to reflect correct indexing
@CLAassistant
Copy link

CLAassistant commented Oct 8, 2024

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants