Skip to content

MDMM hardware-aware metrics (FPGA + PACA), tests and docs#46

Open
ArghyaRanjanDas wants to merge 3 commits into
cern-nextgen:devfrom
ArghyaRanjanDas:add_hardware_aware_metrics
Open

MDMM hardware-aware metrics (FPGA + PACA), tests and docs#46
ArghyaRanjanDas wants to merge 3 commits into
cern-nextgen:devfrom
ArghyaRanjanDas:add_hardware_aware_metrics

Conversation

@ArghyaRanjanDas

Copy link
Copy Markdown
Contributor

Summary

  • Add FPGAAwareSparsity and PACAPattern MDMM metrics to both the Keras and Torch backends, ported onto dev's per-backend layout. Supersedes MDMM tests and hardware-aware metric functions #37, which was built on the old single pruning_methods/ tree and targeted main.
  • Fix the Pydantic serialization warnings reported on MDMM tests and hardware-aware metric functions #37 (the constraint_type enum and the discriminated pruning_parameters union). Root cause: the MDMM enum fields defaulted to bare strings, and Pydantic does not validate/coerce defaults, so the runtime value stayed a str. They now default to enum members.
  • Move metric validation into MDMMPruningModel (ge/le + Literal) and drop the per-metric asserts. PACA auto-forces Equality/target 0 via a model_validator, so the MDMM layer stays metric-agnostic.
  • Run pattern selection on-graph (keras.ops bit-pack + bincount, native torch.unique, no NumPy round-trip of the weight); centralise distance/layout/resource constants in core/constants.py; fix the get_layer_sparsity float/int dtype bug on the TF backend.
  • Add config_mdmm_fpga.yaml, config_mdmm_paca.yaml, the mdmm_fpga_config() / mdmm_paca_config() helpers, and document the new metric types in reference.md and getting_started.md.

Test plan

  • cd tests && pytest test_mdmm.py test_mdmm_metrics.py (Keras / TensorFlow)
  • cd tests && KERAS_BACKEND=torch pytest test_mdmm.py test_mdmm_metrics.py (Keras / Torch)
  • cd tests && KERAS_BACKEND=torch pytest test_torch_mdmm.py (native Torch)
  • Load config_mdmm_fpga.yaml and config_mdmm_paca.yaml, call model_dump(), confirm no PydanticSerializationUnexpectedValue warnings.
  • Existing methods unaffected: pytest test_pdp.py test_ap.py.

Notes for review

  • Most of the MDMM tests and hardware-aware metric functions #37 inline comments are already satisfied by dev's restructure: metric_type is read off the config and resolved via the metric registry, and the MetricType / ConstraintType enums already exist. This PR keeps that flat-params + registry approach rather than nested metric models.
  • The patterns.py no-op OIHW->OIHW permute is replaced with real per-backend layout handling (Keras kernels are HWIO, Torch OIHW), and the convert_to_numpy pattern step is gone.
  • The shipped FPGA config uses Equality/target 0; happy to change the default constraint if you prefer a different target for structured group-sparsity.

Add the two hardware-aware sparsity metrics for MDMM to both the Keras and Torch
backends, on dev's per-backend layout:

- FPGAAwareSparsityMetric: fraction of zero DSP/BRAM weight groups, modelling FPGA
  resource packing (rf / precision / bram_width).
- PACAPatternMetric: drives conv kernels onto a small set of dominant binary
  patterns. Pattern selection stays on-graph (keras.ops bit-pack + bincount /
  native torch.unique, no NumPy round-trip of the weight) and exposes a projection
  mask applied during fine-tuning through a generic, metric-agnostic hook in MDMM.

Data model (MDMMPruningModel):
- New metric_type values + flat Optional fields (precision, target_resource,
  bram_width, num_patterns_to_keep, beta, distance_metric) validated by Pydantic
  (ge/le + Literal); a model_validator forces PACA -> Equality / target 0.
- Fix the reported Pydantic serialization warnings (constraint_type 'Equality' and
  the discriminated pruning_parameters union): enum fields now default to enum
  members instead of bare strings, which Pydantic does not coerce on defaults.

Also: distance/layout/resource constants centralised in core/constants.py, the
get_layer_sparsity float/int dtype fix on the TF backend, and config_mdmm_fpga.yaml
+ config_mdmm_paca.yaml with mdmm_fpga_config() / mdmm_paca_config() helpers.
- test_mdmm.py / test_mdmm_metrics.py: Keras MDMM, FPGA DSP/BRAM math, PACA pattern
  selection + projection masking, full phase cycles, Pydantic validation, and a
  regression test that MDMM configs serialize without Pydantic warnings.
- test_torch_mdmm.py: native-torch MDMM + metrics.
- run_tests.sh: run the new files under both tensorflow and torch backends.
- reference.md: MDMM metric-type overview + FPGAAwareSparsity and PACAPatternSparsity
  parameter tables.
- getting_started.md: constraint-based MDMM usage with the hardware-aware configs.
@nastiapetrovych

Copy link
Copy Markdown
Collaborator

Add these tests to run_tests.sh

pytest test_mdmm.py test_mdmm_metrics.py
KERAS_BACKEND=torch pytest test_mdmm.py test_mdmm_metrics.py
KERAS_BACKEND=torch pytest test_torch_mdmm.py

@nastiapetrovych

Copy link
Copy Markdown
Collaborator

Hi @ArghyaRanjanDas, thank you for your updated PR.

I reviewed it successfully and left you couple of comments on what should be changed before we merge it to the dev branch. Let me know if you have any questions regarding it.

damping: 1.0
use_grad: false
constraint_lr: 1.0e-3
# PACAPatternMetric-specific

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarification about PACAPatternMetric-specific values; are these values result in the best results?

@ArghyaRanjanDas ArghyaRanjanDas Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were the values (hand-tuned) that worked best for the SmartPixels. Do you think it would be better to scan for the best hyperparameters?

If Yes, Kindly let me know if you also have any dataset in mind, that you would like this to be tuned to.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say, that in this situation it would be nice just to leave the best values based on your point of view. If they are the same, leave them as it's.


def convert_conv_layout(w, src, dst=CANONICAL_CONV_LAYOUT):
"""Transpose a 4D conv weight from `src` to `dst` layout (no-op if already equal)."""
if src == dst:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not to make just one condition:
if src =! dst or perm != (0, 1, 2, 3):
and return w in the end instead?

@ArghyaRanjanDas ArghyaRanjanDas Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure! So something like this?

def convert_conv_layout(w, src, dst=CANONICAL_CONV_LAYOUT):
    perm = _perm(src, dst)
    if src != dst or perm != (0, 1, 2, 3):
        return ops.transpose(w, perm)
    return w

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, perfect:)


def _kernel_pattern_distances(kernel_patterns, kernels, dominant_patterns, distance_metric):
"""Distance from every kernel to every dominant pattern. -> (M_kernels, alpha)."""
tk = ops.cast(kernel_patterns, kernels.dtype) # (M, K) binary support of kernels

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these namings are slightly confusing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh okay :)
I will name them something better.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you a lot!

dot = ops.sum(k_e * projected, axis=-1)
denom = ops.norm(k_e, axis=-1) * ops.norm(projected, axis=-1) + keras.backend.epsilon()
return 1.0 - dot / denom
raise ValueError(f"Unsupported distance metric: {distance_metric!r}")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no need in value error, as pydantic will raise it:
distance_metric: Optional[Literal["hamming", "valued_hamming", "cosine"]] = Field(default=None)

_METRIC_REGISTRY = {
"UnstructuredSparsity": UnstructuredSparsityMetric,
"StructuredSparsity": StructuredSparsityMetric,
"FPGAAwareSparsity": FPGAAwareSparsityMetric,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as torch and keras mdmm implementations share the same constraints, metric registry and functions ( _compute_hard_mask), why not to reorganise it in the way that modules reuse that shared code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the principle, the backend is split into two. But I think all the other method also does that, and to unify them into one keras backend that would require a considerable amount of repo-wide refactoring?

@ArghyaRanjanDas ArghyaRanjanDas Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should really post-pone this large refactoring, unless we decide to start moving everything to keras.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense for now

PACA_DISTANCE_METRICS = (DISTANCE_HAMMING, DISTANCE_VALUED_HAMMING, DISTANCE_COSINE)

# FPGAAwareSparsityMetric target hardware resources
TARGET_RESOURCE_DSP = "DSP"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, why not to make FPGA_TARGET_RESOURCES_REGISTRY, etc?

@@ -67,6 +67,8 @@ class ActivationPruningModel(BasePruningModel):
class MetricType(str, Enum):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be as a general metric class, and these subclasses inherit from it and reimplement some methods

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also sounds great! Implementing that

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

return zero_bram.sum() / float(bram_norms.numel())


class PACAPatternMetric:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why these metric classes aren't Pydantic?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This are compute objects as the metric holds live tensor and bound method. So I think we would have to allow arbitrary_types_allowed=True if we want to move to Pydantic, which I think would not be good code policy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also during config parsing, Pydantic is already validating by the time MDMM.build constructs a metric, every value has passed the Literal/ge/le checks in MDMMPruningModel. So validating again inside the class would be double validation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if we do that per-metric config sub-models from the other thread, there will be effectively a Pydantic class per metric where it owns and validates the parameters. And the runtime class will just then consume those validated values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kindly let me know how that sounds to you or if I am missing anything

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it wouldn't be a problem if we make a good module structure planning -> we can discuss it via meet

@@ -0,0 +1,72 @@
# file: /src/pquant/configs/config_mdmm_fpga.yaml

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too much redundant parameters in config files

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure Happy to trim them. these two were clones from the config_mdmm.yaml to stay consistent and trimming them might make it a bit confusing for the user (ommited = default or ommited = forgotten). Do you think we should trim just these two or all of them? the minimal versions should be ~15 lines

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can make separate variation of files, including different mdmm parameters for different settings? All others blocks (training, hpo, quantization) should stay the same within the files.

pruning_parameters:
pruning_method: mdmm
enable_pruning: true
disable_pruning_for_layers: [] # Disable pruning for these layers, even if enable_pruning is true

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, too much redundant parameters

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

@ArghyaRanjanDas

Copy link
Copy Markdown
Contributor Author

Add these tests to run_tests.sh

pytest test_mdmm.py test_mdmm_metrics.py
KERAS_BACKEND=torch pytest test_mdmm.py test_mdmm_metrics.py
KERAS_BACKEND=torch pytest test_torch_mdmm.py

Hi, you might have missed it, they are already added, lines 7–9 of tests/run_tests.sh‎

@nastiapetrovych

Copy link
Copy Markdown
Collaborator

Thanks @ArghyaRanjanDas for these modifications.

Yes, I have missed run_test.sh changes, thanks for pointing them out. If you need to discuss any of these comments, feel free to reach out to me:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature new feature added fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants