Feature Election contribution

[christofilojohn]

Fixes
Implemented all the changes you requested on file format, tests etc. black-check, isort-check, flake8 pass.

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).
• Breaking change (fix or new feature that would cause existing functionality to change).
• New tests added to cover the changes.
• Quick tests passed locally by running ./runtest.sh.
• In-line docstrings updated.
• Documentation updated.

[greptile-apps]

_{14 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

1. nvflare/app_opt/feature_election/feature_election.py, line 265-266 (link)

   logic: Feature names are being overwritten on each client iteration - this should be set once before the loop to maintain consistency across all clients

_{14 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[christofilojohn]

Hello @chesterxgchen, I didn't do the proper formatting with black, flake8 and isort after applying some greptile recommendations and that caused the tests to fail. Everything ran locally on the last commit, so I expect the tests to be ok now.
Apologies for the inconvenience,
Best regards,
Ioannis

[greptile-apps]

Additional Comments (1)

1. examples/advanced/feature_election/README.md, line 90 (link)

   syntax: Missing parameter in function call - client_id should be self.client_id or obtained from fl_ctx

_{14 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

1. nvflare/app_opt/feature_election/executor.py, line 81 (link)

   style: The feature_names parameter is accepted but never used - consider removing or implementing its usage

   _{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

_{14 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (10)

1. examples/advanced/feature_election/client.py, line 162 (link)

   logic: The fallback client_id = int(match.group()) - 1 if match else 0 could result in negative client IDs if the parsed number is 0, which may cause issues with data loading. Should there be validation to ensure client_id is non-negative?

2. nvflare/app_opt/feature_election/README.md, line 328 (link)

   style: Broken internal link - LICENSE file should be referenced as ../../../LICENSE or use absolute GitHub path

3. examples/advanced/feature_election/prepare_data.py, line 128 (link)

   logic: Edge case: if num_clients > len(class_indices), np.random.choice with replace=False will fail

4. examples/advanced/feature_election/prepare_data.py, line 322 (link)

   style: Using pop() modifies the original dataset_kwargs dictionary which may cause unexpected side effects if called multiple times

   _{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

5. nvflare/app_opt/feature_election/feature_election.py, line 223-227 (link)

   logic: Potential issue with dirichlet splitting when proportions creates splits that don't cover all indices. If len(splits) < num_clients, some clients may get empty data or indices could be lost. Should there be validation to ensure all clients receive data in dirichlet splitting?

6. nvflare/app_opt/feature_election/feature_election.py, line 206-209 (link)

   logic: Random and sequential splitting can create uneven splits due to integer truncation. Last client may get more/fewer samples than expected.

7. nvflare/app_opt/feature_election/feature_election.py, line 272 (link)

   style: Calling method without checking return format - ensure perform_feature_selection() always returns tuple of (mask, scores)

   _{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

8. nvflare/app_opt/feature_election/controller.py, line 231 (link)

   logic: Variable best_score is assigned but never used after extraction from tuning history

9. nvflare/app_opt/feature_election/controller.py, line 325-327 (link)

   logic: Empty client_selections should return appropriate-sized zero array rather than empty array to maintain shape consistency with feature dimensions. What should be the expected feature dimension when no client selections are available?

10. nvflare/app_opt/feature_election/controller.py, line 389 (link)

    logic: np.partition can produce unstable results when multiple values equal the cutoff threshold, potentially causing inconsistent feature selection across runs

_{14 files reviewed, 10 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (4)

1. nvflare/app_opt/feature_election/feature_election.py, line 273 (link)

   syntax: Variable result is undefined - should be selected_mask, feature_scores

2. nvflare/app_opt/feature_election/executor.py, line 236 (link)

   logic: PyImpetus fallback also uses unscaled data, should use X_scaled for consistency

3. nvflare/app_opt/feature_election/controller.py, line 327 (link)

   logic: getattr(self, "n_features", 0) assumes n_features attribute exists. This could return an empty array if n_features is never set, potentially causing issues downstream. Should n_features be initialized in init or determined from first client response?

4. tests/unit_test/app_opt/feature_election/test.py, line 184-186 (link)

   logic: This assertion assumes intersection_features will always equal the number selected at FD=0, but this may not hold if the implementation changes aggregation logic. Is this assumption about intersection behavior guaranteed by the Feature Election algorithm specification?

_{14 files reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

1. nvflare/app_opt/feature_election/controller.py, line 327 (link)

   logic: Using getattr(self, "n_features", 0) may return 0 if n_features is not set, potentially creating an empty boolean array. Should n_features be initialized in __init__ or derived from client data? Should n_features be determined from the first client's data dimensions or initialized as a required parameter?

_{14 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

1. nvflare/app_opt/feature_election/controller.py, line 327 (link)

   logic: The getattr(self, "n_features", 0) assumes controller has n_features attribute which is never set. This could return an empty boolean array if no client selections exist. Consider determining n_features from client data or handling this case differently. Should n_features be determined from the first client's data or passed as a constructor parameter?

_{14 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[christofilojohn]

Greptile comment made sense so I implemented the change.

[greptile-apps]

Additional Comments (1)

1. nvflare/app_opt/feature_election/controller.py, line 336 (link)

   logic: Using getattr(self, "n_features", 0) will return 0 when n_features is None, creating an empty mask when it should use the actual feature count

_{14 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Greptile's behavior is changing!

From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section.

_{This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR".}

[chesterxgchen]

Additional Comments (1)

1. nvflare/app_opt/feature_election/controller.py, line 336 (link)
   logic: Using getattr(self, "n_features", 0) will return 0 when n_features is None, creating an empty mask when it should use the actual feature count

14 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

need to check the n_features attribute available or not, if not, raise exception ?

[christofilojohn]

@chesterxgchen I will change it, it is an edge case that was not handled properly. I would like to ask if there is a way to optimize my code to pass the blossom-cli before running on actions. Is there a local way to prepare it?
Thank you, best wishes for the new year,
Ioannis

[chesterxgchen]

To run test locally ./run_tests.sh will run unit tests There is also integration test you can run locally Blossom CI run the same test on blossom cluster Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Ioannis Christofilogiannis ***@***.***> Sent: Wednesday, December 31, 2025 4:27:58 AM To: NVIDIA/NVFlare ***@***.***> Cc: Chester Chen ***@***.***>; Mention ***@***.***> Subject: Re: [NVIDIA/NVFlare] Feature Election contribution (PR #3876) [https://avatars.githubusercontent.com/u/71899248?s=20&v=4]christofilojohn left a comment (NVIDIA/NVFlare#3876)<#3876 (comment)> @chesterxgchen<https://github.com/chesterxgchen> I will change it, it is an edge case that was not handled properly. I would like to ask if there is a way to optimize my code to pass the blossom-cli before running on actions. Is there a local way to prepare it? Thank you, best wishes for the new year, Ioannis — Reply to this email directly, view it on GitHub<#3876 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAD5FQ5W6UBPE3JACJQEMO34EO6M5AVCNFSM6AAAAACN6EQRV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTOMBSGEYTGOJSGM>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[christofilojohn]

Thanks for the information,
I made the change and passed the tests locally, I don't expect any errors on the integration tests as this module doesn't change the existing nvflare codebase.

[greptile-apps]

Additional Comments (2)

1. examples/advanced/feature_election/prepare_data.py, line 128 (link)

   logic: np.random.choice with replace=False will fail if len(class_indices) > num_clients. Should check this condition or use replace=True

2. examples/advanced/feature_election/prepare_data.py, line 189 (link)

   logic: Setting proportions[-1] directly could result in negative values if the sum of previous proportions exceeds len(idx_k) due to integer conversion. Should use max(0, len(idx_k) - proportions[:-1].sum())

_{13 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}